-
Notifications
You must be signed in to change notification settings - Fork 188
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
Standardize verb usage within the module #228
Conversation
Definitely interested in hearing thoughts on this: |
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
What's the difference between |
Currently in PowerShellForGitHub/GitHubLabels.ps1 Lines 381 to 385 in 742b853
while Set-GitHubLabel will take in an array of labels and either create them if there isn't an existing label with that name, or change the color of the label based on the value in the label if it does exist. It will also remove any labels that are in the repo but not in the array.PowerShellForGitHub/GitHubLabels.ps1 Lines 502 to 507 in 742b853
The functionality as described in With this change, |
Definitely agree with |
|
I've just seen |
Interesting observation. I could see making that change (and aliasing it back to
Thanks for bringing this up. |
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
If you ever wanted to get all the verb descriptions, you can just run |
Nope. That only tells you the Name and Grouping. It doesn't give additional context/description of its expected usage. That link I referenced earlier was the first time I found a good explanation regarding the intended usage of the different verbs. |
I think our documentation got moved and the Approved Verbs page (which itself was a reconstruction of the archived page you linked @HowardWolosky) lost its search engine rank. It's now at https://docs.microsoft.com/en-us/powershell/scripting/developer/cmdlet/approved-verbs-for-windows-powershell-commands. Possibly Output on my machine in PS 7.1.0-preview.3:
|
Ah, indeed. I had been running |
I'll be merging this change in later tonight pending any further feedback. |
GitHubLabels.ps1
Outdated
@@ -551,7 +552,7 @@ function Set-GitHubLabel | |||
removed (and thus unassigned from existing Issues) and then the new one created. | |||
|
|||
.EXAMPLE | |||
Set-GitHubLabel -OwnerName Microsoft -RepositoryName PowerShellForGitHub -Label @(@{'name' = 'TestLabel'; 'color' = 'EEEEEE'}, @{'name' = 'critical'; 'color' = 'FF000000'; 'description' = 'Needs immediate attention'}) | |||
Initialize-GitHubLabel -OwnerName Microsoft -RepositoryName PowerShellForGitHub -Label @(@{'name' = 'TestLabel'; 'color' = 'EEEEEE'}, @{'name' = 'critical'; 'color' = 'FF000000'; 'description' = 'Needs immediate attention'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Switch
?
Specifies an action that alternates between two resources, such as to change between two locations, responsibilities, or states
or Redo
?
Resets a resource to the state that was undone
or Use
?
Uses or includes a resource to do something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the suggestions there, but none of those resonate.
Switch
- Of your suggestions, I think this was the closest, but still I don't think it's as clear as Initialize
.
Redo
- implies that there was a previous state, but there isn't necessarily a previous label state for a repo
Use
- We're not using the labels to do something...we're changing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to blow the question out here, but what are the two API endpoints the different functions hit?
If they really have to be different commands with different verbs, I would say:
Update
->Edit
(that's the approved replacement forUpdate
) since it "Modifies existing data by adding or removing content."Set
stays the same, since it "Replaces data on an existing resource or creates a resource that contains some data"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoints are identical. Initialize
is an old, historical wrapper/helper function that existed before I took the module over. It calls Get
to get the current set of labels. Then it loops through the input array, either adding labels (New
) if they didn't already exist or updating (Set
) the colors for labels that already existed, and then deleting (Remove
) any labels that were previously there but not in the new array.
The functionality doesn't match that of any GitHub provided endpoint...it's just a helper function that had been created by the previous owner. I'm just trying to name it correctly.
I wanted to avoid changing over to Edit
, because then I'd be changing every Set
call over to Edit
since they all work similarly which would be a really big breaking change unless I aliased-back to Set-*
as well. I figured I'd standardize on Get
/Set
/New
/Remove
, as that all felt rather natural and was mostly what we were already doing. I'll mull over if I should do the change Edit
and alias back to Set
. I definitely can't migrate to Edit
without the alias right now.
d2761aa
to
997fbf1
Compare
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
This attempts to rectify some improper verb usage in the module based on the explanations of intended verb usage from [previous PowerShell documentation](https://web.archive.org/web/20171222220053/https://msdn.microsoft.com/en-us/library/windows/desktop/ms714428(v=vs.85).aspx). * We're standardizing on the following pattern for most object actions: `Get` / `Set` / `New` / `Remove` * We will continue to alias `Remove-*` as `Delete-*`. * When possible, this change attempts to avoid breaking changes by re-aliasing the newly named functions with their previous names. This was not possible in one instance, hence this is still a breaking change (although based on telemetry, it should be minimally impacting). Result: * `Update-GitHubCurrentUser` -> `Set-GitHubProfile` `[Alias('Update-GitHubCurrentUser')]` * `Update-GitHubIssue` -> `Set-GitHubIssue` `[Alias('Update-GitHubIssue')]` * `Update-GitHubRepository` -> `Set-GitHubRepository` `[Alias('Update-GitHubRepository')]` * [breaking] `Update-GitHubLabel` -> `Set-GitHubLabel` `[Alias('Update-GitHubLabel')]` * [breaking] `Set-GitHubLabel` -> `Restore-GitHubLabel` `<no alias due to above>` Changing an existing label has much more regular usage than replacing all of the labels in a repository, hence allowing the _new_ `Set-GitHubLabel` to keep the alias of `Update-GitHubLabel`. Our usage of the `Set-*` verb in general is a bit arguable based on the documentation ... in theory `Edit-*` might be a better fit since we're _editing_ aspects of an object as opposed to _replacing_ the entire content of an object. However, I think `Set-*` _feels_ ok in this module. We're _setting_ the state of these objects. Again...arguable, but this is a much smaller breaking change to get to a consistent terminology state.
997fbf1
to
8733380
Compare
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This attempts to rectify some improper verb usage in the module
based on the explanations of intended verb usage from previous
PowerShell documentation.
We're standardizing on the following pattern for most object actions:
Get
/Set
/New
/Remove
We will continue to alias
Remove-*
asDelete-*
.When possible, this change attempts to avoid breaking changes by
re-aliasing the newly named functions with their previous names.
This was not possible in one instance, hence this is still a breaking
change (although based on telemetry, it should be minimally impacting).
Result:
Update-GitHubCurrentUser
->Set-GitHubProfile
[Alias('Update-GitHubCurrentUser')]
Update-GitHubIssue
->Set-GitHubIssue
[Alias('Update-GitHubIssue')]
Update-GitHubRepository
->Set-GitHubRepository
[Alias('Update-GitHubRepository')]
New-GitHubAssignee
->Add-GitHubAssignee
[Alias('New-GitHubAssignee')]
Update-GitHubLabel
->Set-GitHubLabel
[Alias('Update-GitHubLabel')]
Set-GitHubLabel
->Initialize-GitHubLabel
<no alias due to above>
Changing an existing label has much more regular usage than replacing
all of the labels in a repository, hence allowing the new
Set-GitHubLabel
to keep the alias ofUpdate-GitHubLabel
.Our usage of the
Set-*
verb in general is a bit arguable based onthe documentation ... in theory
Edit-*
might be a better fit sincewe're editing aspects of an object as opposed to replacing the entire
content of an object. However, I think
Set-*
feels ok in this module.We're setting the state of these objects. Again...arguable, but this
is a much smaller breaking change to get to a consistent terminology state.
Issues Fixed
None
References
PowerShell documentation on verb usage
Checklist
is reporting back clean.
If desired, ensure your name is added to our Contributors list