Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - updates to README for 1.0.0 release #529

Closed
wants to merge 23 commits into from

Conversation

rkeithhill
Copy link
Collaborator

I added a markdown linter to the list of recommended VSCode extensions. I cleaned up most of the linter warnings in the README. Still work to do here in terms of the branch/version listings. I'd also like to get the Travis URLs for the build table.

Completely open to suggestions here as I tweaked the format (mainly the beginning) just a bit.

@rkeithhill rkeithhill mentioned this pull request Jan 13, 2018
README.md Outdated
## Versions
### Build status

| AppVeyor (Windows) | Travis (Linux) | Travis (macOS) | Code Coverage Status |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe it's possible to have separate Linux/macOS builds on Travis, so those columns could probably be combined.

That said, since there are subsections below for v0 and v1, I was thinking we could just put the build status for each right under their headings. This is as far as I got yesterday:
image

Also, tangent: Coveralls doesn't appear to support multiple/merged coverage reports, so we may want to use a different service if we wish to differentiate coverage by platform.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkeithhill did you have any thoughts on this? I am tentatively planning to pull the relevant parts from this PR into an update targeting master, before releasing v0.7.2.

README.md Outdated
PowerShell Core 6.x, both of which support writing prompt strings using [ANSI escape sequences][ansi-esc-code].
On Windows, support for [Console Virtual Terminal Sequences][console-vt-seq] was added in Windows 10 version 1511.
Consequently this version of posh-git introduces BREAKING changes with 0.x which including dropping support
for Windows PowerShell version 2, 3 and 4 since these versions of PowerShell do not run on platforms that
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually true that PowerShell pre-5 doesn't support prompts with ANSI escape sequences? I haven't tried, but I see no reason it wouldn't work with a proper host. Furthermore, 1.0 should still work on hosts without ANSI support.

It feels like you're blaming ANSI for the breaking changes, when they're mostly motivated (I think?) by improving the user experience (settings refactor) and making our lives easier dev-wise (newer languages features).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it is v5 (or v5.1) that adds the $host.UI.SupportsVirtualTerminal property we use to set the AnsiConsole setting. But I suppose someone could be using ConEmu or a shell like that. That said, posh-git 1.x won't run on anything less than v5 because of our use of PS classes.

RE blaming ANSI - I'm fine with rewording that to shift the emphasis to the settings restructuring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded.

README.md Outdated

### Active branches

TODO: @dahlbyk Need to update for the new maintenance branch and moving v1 to master (and probably nuke develop, right?). Maybe we integrate this info into the appropriate v0.x / v1.x sections above?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been pondering this. What if we just "renamed" master to v0, develop to v1, and started working with a v2 branch when the time comes? It would simplify "how do I link to v0's README?"-type questions, as well as targeting of PRs (is this change for v0 or v1?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking you'd create a v0.x branch off of master, merge develop back to master and nuke develop for now. But you've got far more experience in this area than I. I'll defer to your judgement.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing special about master other than it being the repo's default branch, which we can change. I suppose it might mess with existing clones a bit if master disappeared, though.

I'll move forward with branch rearrangement as soon as v0.7.2 ships.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.md Outdated

2. Script execution policy must be set to either `RemoteSigned` or `Unrestricted`.
1. On Windows, script execution policy must be set to either `RemoteSigned` or `Unrestricted`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these Markdown lint rules be tuned? One of the key objectives of Markdown is to be readable as plain text, so I prefer to use real numbering.

I also prefer inline links over reference links, especially for the CHANGELOG issue/PR links, though that preference isn't particularly strongly held.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can suppress md lint rules:

  "markdownlint.config": {
    "MD013": false
  },

I was of the same opinion on the 1., 1., 1. lists but was eventually convinced that the vast majority will read it rendered and there's a benefit to not having to renumber (and keep numbering correct) on numbered lists. That said, I really had no choice - wasn't my repo and had to follow the rules. :-) I'm fine with suppressing this warning (or just living with squiggles in the md).

RE ref links, for a CHANGLELOG I definitely like inline. For README's I like having the links organized at the bottom - kind of like a bibliography. :-) Most of the other OSS projects I work on do it this way. But I can put it back to inline if you'd like.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the linter won't complain about preferred style on all our (two) markdown files, I'm good. Reference links on readme is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the settings to not complain about numbered lists 1., 2., 3., etc and reverted the numbering change.

README.md Outdated

```powershell
PowerShellGet\Install-Module posh-git -Scope CurrentUser
Install-Module posh-git -Scope CurrentUser -Force
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Force to get latest version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To force side-by-side install. If you already have 0.7.1 installed, Install-Module will complain IIRC. Update-Module posh-git would work if you installed from the PSGallery. Or just use Install-Module with -Force.

WARNING: Version '0.7.1' of module 'posh-git' is already installed at
'C:\Users\Keith\Documents\WindowsPowerShell\Modules\posh-git\0.7.1'. To install version '1.0.0-beta1', run
Install-Module and add the -Force parameter, this command will install version '1.0.0-beta1' in side-by-side with
version '0.7.1'.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to your experience here.

README.md Outdated

If you are on PowerShell version 3 or 4, you will need to install the [Package Management Preview for PowerShell 3 & 4](https://www.microsoft.com/en-us/download/details.aspx?id=51451) in order to run the command above.
- If you are asked to trust packages coming from the PowerShell Gallery.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could PowerShell Gallery troubleshooting tips be punted to a wiki page (here or a more canonical source)?
  2. If we keep them here, this rephrased sentence should be joined to the following line with a comma.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely could punt. Wasn't sure about keeping this so I can be trivially talked into removing it. :-)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are common hang-ups, and posh-git is often the only PS module a dev may have installed, I want to make sure guidance is available. Open to thoughts on where that would best live.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trust issue is pretty common so I recommend we keep the troubleshooting help here.

README.md Outdated
[![develop build coverage](https://coveralls.io/repos/github/dahlbyk/posh-git/badge.svg?branch=develop)](https://coveralls.io/github/dahlbyk/posh-git?branch=develop)

[![Join the chat at https://gitter.im/dahlbyk/posh-git](https://badges.gitter.im/dahlbyk/posh-git.svg)](https://gitter.im/dahlbyk/posh-git?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
[![posh-git on Chocolatey](https://img.shields.io/chocolatey/dt/poshgit.svg)](https://chocolatey.org/packages/poshgit/)
Copy link
Owner

@dahlbyk dahlbyk Jan 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentionally dropped? I was thinking we'd add a PowerShell Gallery badge, too:
PowerShell Gallery

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added in the PSGallery badge. I had removed the Choco badge since the choco install instructions were removed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still leaning towards adjusting the Chocolatey installer for 1.x to wrap PowerShellGet, in which case I would suggest we keep basic instructions (with a note about it's relationship to PS Gallery) and the badge.

@rkeithhill
Copy link
Collaborator Author

@dahlbyk This needs some additional info on the updated $GitPromptSettings - also wanted to provide a link to https://www.w3schools.com/colors/colors_names.asp and discuss the additional Write-*Status commands. Did you already take this and modify it or is it OK for me to continue updating this PR?

@dahlbyk
Copy link
Owner

dahlbyk commented Jan 18, 2018

Did you already take this and modify it or is it OK for me to continue updating this PR?

Please continue, I won't have a chance to do much here until early next week.

@rkeithhill
Copy link
Collaborator Author

Getting close to being done on this. However, I'm wondering if we should use a few PNGs to display properly colored prompt text? Simply using the powershell syntax colorizer is not coloring properly. That said, I don't know if you want a half-dozen or so PNGs added to the repo. I guess we could put them in the Wiki and then link to them. Thoughts?

Another approach is to true to embed HTML style elements in the markdown file to get the right colors but I think that would likely make the markdown quite messy.

Add MD024 to markdown linter disable list.  With CHANGELOG we have multiple headers with same name e.g. Added, Changed, Removed, Fixed.
@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Jan 30, 2018

I think I'd like to drop in either a formatted text section or ideally a PNG in the Customization variables section:

image

Thoughts? The PNG file size is 137KB (and from beta1 - needs to be updated for beta2). We could put it in a doc\images dir or we could fudge and grab the blog URL for it from this issue (once it's updated).

As text it is less nice:

AnsiConsole                            : True
DefaultColor                           : ForegroundColor: <default>, BackgroundColor: <default>
BranchColor                            : ForegroundColor: �[106m  �[0m Cyan, BackgroundColor: <default>
IndexColor                             : ForegroundColor: �[42m  �[0m DarkGreen, BackgroundColor: <default>
WorkingColor                           : ForegroundColor: �[41m  �[0m DarkRed, BackgroundColor: <default>
StashColor                             : ForegroundColor: �[101m  �[0m Red, BackgroundColor: <default>
ErrorColor                             : ForegroundColor: �[101m  �[0m Red, BackgroundColor: <default>
BeforeText                             : Text: '�[93m [�[0m',	 ForegroundColor: �[103m  �[0m Yellow, BackgroundColor: <default>
...

README.md Outdated
[av-master-site]: https://ci.appveyor.com/project/dahlbyk/posh-git/branch/master
[av-master-img]: https://ci.appveyor.com/api/projects/status/eb8erd5afaa01w80/branch/master?svg=true&pendingText=master%20%E2%80%A3%20pending&failingText=master%20%E2%80%A3%20failing&passingText=master%20%E2%80%A3%20passing

[tv-develop-img]: https://travis-ci.org/dahlbyk/posh-git.svg?branch=develop
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link doesn't seem to be giving an updated/correct image. Image indicates failure but when you click on it, the page indicates success.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curiously, Travis doesn't seem to support branch-specific links. travis-ci/travis-ci#5024

I'm expecting this link will be correct once v1 becomes the default branch here.

@dahlbyk
Copy link
Owner

dahlbyk commented Feb 4, 2018

I won't have a chance to do much here until early next week.

lol

I'm wondering if we should use a few PNGs to display properly colored prompt text? Simply using the powershell syntax colorizer is not coloring properly. That said, I don't know if you want a half-dozen or so PNGs added to the repo. I guess we could put them in the Wiki and then link to them. Thoughts?

Another approach is to true to embed HTML style elements in the markdown file to get the right colors but I think that would likely make the markdown quite messy.

An image of the expected prompt display, with appropriate alt text, seems reasonable. I agree that embedded HTML style would get too messy.

Including that same image in a wiki page, and letting GitHub host the image, seems preferable to including in the repo.

I think I'd like to drop in either a formatted text section or ideally a PNG in the Customization variables section…

Thoughts? The PNG file size is 137KB (and from beta1 - needs to be updated for beta2). We could put it in a doc\images dir or we could fudge and grab the blog URL for it from this issue (once it's updated).

As text it is less nice…

I'm content to have the more complete explanation of this on the wiki. And we should definitely strip any ANSI color sequences from the README.

@rkeithhill
Copy link
Collaborator Author

Take a look at the prompt images (first two). https://github.com/dahlbyk/posh-git/wiki/Customizing-Your-PowerShell-Prompt

They are a bit big due to me taking them on my high DPI monitor. Do you think these are too big?

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Feb 11, 2018

Hmm, I think I'm going to need to redo some of these images:

The DarkRed we use for the WorkingDir status is too dark. There is not enough contrast between it and the black background. This is the default/legacy color config I'm using (colortool cmd-legacy). Thoughts?

UPDATE: I think the issue is largely due to the fact that the web page has a whole lot of white. And that little swatch of black with DarkRed text doesn't look very good in that context. In the actual console, it doesn't look that bad to me. So for this image, I boosted the Red intensity from 128 to 192. The image above now reflects that change.

@rkeithhill
Copy link
Collaborator Author

BTW I think this README is close to being done. The tweaks that need to be made are in the version specific sections where we reference branches. And that only needs to change once we do the branch swaparoo.

@rkeithhill rkeithhill mentioned this pull request Feb 20, 2018
13 tasks
@dahlbyk
Copy link
Owner

dahlbyk commented Apr 20, 2018

Waiting on this until v0.7.2 is out.

@dahlbyk dahlbyk added this to the v1.0 milestone Apr 20, 2018
@dahlbyk dahlbyk mentioned this pull request Apr 20, 2018
@dahlbyk dahlbyk changed the base branch from develop to master April 20, 2018 05:32
@dahlbyk
Copy link
Owner

dahlbyk commented May 5, 2018

Remove develop branch from build results

👍 can we add a row for v0 status? https://github.com/dahlbyk/posh-git/blame/v0/README.md#L20-L21

@rkeithhill
Copy link
Collaborator Author

Working on that right now.

@rkeithhill
Copy link
Collaborator Author

OK, I feel pretty good about this file now. The order of the sections is certainly debatable and should be easy to move around. But I "think" this is good enough to merge to master - unless you want me to re-order sections before we do that.

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented May 5, 2018

One option is to perhaps re-order this:

- [Overview]
- [Git status summary information]
- [Customization variables]
- [Versions]
- [Installation]
- [Using posh-git]
- [Customizing the posh-git prompt]
- [Based on work by]

to

- [Overview]
- [Versions]
- [Installation]
- [Using posh-git]
- [Git status summary information]
- [Customization variables]
- [Customizing the posh-git prompt]
- [Based on work by]

Thoughts?

@dahlbyk
Copy link
Owner

dahlbyk commented May 6, 2018

I like this:

  • [Overview]
  • [Versions]
  • [Installation]
  • [Using posh-git]
  • [Git status summary information]
  • [Customization variables]
  • [Customizing the posh-git prompt]
  • [Based on work by]

There's a conflict with master, too.

@rkeithhill
Copy link
Collaborator Author

I made the re-order changes in PR #572. Seemed easier to just re-create the PR from a fresh branch off of master than to figure out the right commit to squash to.

@rkeithhill rkeithhill closed this May 6, 2018
@rkeithhill rkeithhill deleted the rkeithhill/update-readme branch May 14, 2018 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants