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

UseApprovedVerbs.md: Improving Documentation #1294

Merged
merged 3 commits into from
Jul 23, 2019
Merged

UseApprovedVerbs.md: Improving Documentation #1294

merged 3 commits into from
Jul 23, 2019

Conversation

Banner-Keith
Copy link
Contributor

PR Summary

I have come to this documentation a few times in the past. Get-Verb is very helpful, but the additional insight, and examples of unapproved verbs in the Microsoft docs are very helpful and make it easier to decide on a suitable replacement for an unapproved verb. I think it would be helpful to have a link to that documentation here.

The contribution guidelines say "Submit your own fixes or features as a pull request but please discuss it beforehand in an issue if the change is substantial." This change is not substantial and therefore I did not open an issue first.

PR Checklist

I have come to this documentation a few times in the past. Get-Verb is very helpful, but the additional insight, and examples of unapproved verbs in the Microsoft docs are very helpful and make it easier to decide on a suitable replacement for an unapproved verb. I think it would be helpful to have a link to that documentation here.
@@ -8,6 +8,8 @@ All CMDLets must used approved verbs.

Approved verbs can be found by running the command `Get-Verb`.

Additional documentation on approved verbs can be found in the microsoft docs page [Approved Verbs for PowerShell Commands](https://docs.microsoft.com/en-us/powershell/developer/cmdlet/approved-verbs-for-windows-powershell-commands). If you find the verb you are using is unapproved, try searching the page for the approved equivalent. For example, if you search in the documentation for `Read`, `Open`, or `Search` you will find that the approved verb for those situations is `Get`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Looks good to me. Just a minor suggestion to take out the \en-us part of the link so that the page gets displayed in the user's locale.

Suggested change
Additional documentation on approved verbs can be found in the microsoft docs page [Approved Verbs for PowerShell Commands](https://docs.microsoft.com/en-us/powershell/developer/cmdlet/approved-verbs-for-windows-powershell-commands). If you find the verb you are using is unapproved, try searching the page for the approved equivalent. For example, if you search in the documentation for `Read`, `Open`, or `Search` you will find that the approved verb for those situations is `Get`.
Additional documentation on approved verbs can be found in the microsoft docs page [Approved Verbs for PowerShell Commands](https://docs.microsoft.com/en-us/powershell/developer/cmdlet/approved-verbs-for-windows-powershell-commands). If you find the verb you are using is unapproved, try searching the page for the approved equivalent. For example, if you search in the documentation for `Read`, `Open`, or `Search` you will find that the approved verb for those situations is `Get`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I have removed that. Do I need to squash both changes into one commit before the pull request is completed?

Copy link
Collaborator

@bergmeister bergmeister Jul 23, 2019

Choose a reason for hiding this comment

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

You're welcome. No need for squashing, when the PR gets merged all commits will get squashed automatically by GitHub and the final commit message will be the PR title and the description. I will assign a 2nd reviewer to as a best practice and we'll then take care of merging it in. Thanks :-)

@@ -8,6 +8,8 @@ All CMDLets must used approved verbs.

Approved verbs can be found by running the command `Get-Verb`.

Additional documentation on approved verbs can be found in the microsoft docs page [Approved Verbs for PowerShell Commands](https://docs.microsoft.com/powershell/developer/cmdlet/approved-verbs-for-windows-powershell-commands). If you find the verb you are using is unapproved, try searching the page for the approved equivalent. For example, if you search in the documentation for `Read`, `Open`, or `Search` you will find that the approved verb for those situations is `Get`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this could be broken into semantic lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

The second sentence is backwards I think; it reads like:

If you are using an unapproved verb, try searching for its approved equivalent (which you're trying to find)

It might be worded better as:

Some unapproved verbs are documented on the approved verbs page and point to approved alternatives; try searching for the verb you used to find its approved form. For example, searching for Search or Open will lead you to Get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll update that.

@rjmholt
Copy link
Contributor

rjmholt commented Jul 23, 2019

Thanks for your contribution @Banner-Keith

@rjmholt rjmholt merged commit 17e9677 into PowerShell:master Jul 23, 2019
@Banner-Keith
Copy link
Contributor Author

@rjmholt I noticed that the Show documentation hint in the PowerShell extension points at the development branch. That might be because I'm using the preview. Anyway. This change is not in the development branch, just in master. Could we merge it there too?

@bergmeister
Copy link
Collaborator

@Banner-Keith After the release of 1.18.1 we decided to abandon this model of using 2 branches and started using master only. The development branch is only there for legacy purposes and will not be updated any more. The next version of the VS-Code PowerShell extension will start pointing to master, this change was already merged: PowerShell/vscode-powershell#2037
PSScriptAnalyzer 1.18.2 will soon release in the next days and if everything works out well then after that the VS-Code extension will release as well a bit later.
We could merge the changes made in master back to development though if you need them now and don't want to wait?

@Banner-Keith
Copy link
Contributor Author

@bergmeister Thanks for the reply.
It sounds like there is a good solution going forward. I'm okay with waiting. Thanks for explaining how that will work.

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.

3 participants