Reimagining status for the module #253
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HowardWolosky
added
the
enhancement
|
@X-Guardian - you've been quite vocal regarding your feelings of how status is handled in the module. I'd be interested in your thoughts on this change. |
|
Sounds great @HowardWolosky. This remove all the complexity of |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Status for commands was originally added to this module based on my experience with other REST API's where individual commands could easily take 10-20 seconds. Practical usage has shown that most GitHub requests in reality take under one second. The additional work that PowerShell has to do in order to display progress to the user can easily make the overall command take 4-6 times longer than its actual execution time. Therefore, status is being ripped out of this module (for the most part). `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no longer have bifurcating execution paths based on the value of `$NoStatus`. Everything runs synchronously now on the command prompt. * `DefaultNoStatus` has been deprecated. Its value will be ignored. * The `NoStatus` switch has not been removed from the module commands in order to avoid a breaking change. It may be removed in a future update. * `Invoke-GHRestMethod -ExtendedResult` has been updated to include the next page's number and the total number of pages for the REST request. * A new configuration value has been added: `MultiRequestProgressThreshold` `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the user tracking the number of remaining requests for the overall execution of the requested command based on this threshold value. It will only display the progress bar if the number of requets needed meets or exceeds this threshold value. This defaults to 10, and can be disabled with a value of 0. Practical usage has shown that this adds less than a second of additional time to the overall execution of a multi-request command (quite different than the previous status). * `Wait-JobWithAnimation` has been removed since it's no longer used. Fixes microsoft#247
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
HowardWolosky
added a commit
to HowardWolosky/PowerShellForGitHub
that referenced
this pull request
Aug 11, 2020
The `NoStatus` parameter (and `DefaultNoStatus` config value) were deprecated as part of microsoft#253 when we stopped showing status except for multi-page requests that exceeded some minimum number of pages. At that time, `NoStatus` (and `DefaultNoStatus`) were not removed in order to minimize the churn to the module. However, keeping it in is adding unnecessary complexity to the module as we continue to expand what the module can do. This change removes `NoStatu` (and `DefaultNoStatus`) from the module entirely. The only impact that this may cause is with users who are currently using one (or both) of them. This breaking change impact should be minimal, but its best for the breaking change to be part of the coming release which already has a number of other breaking changes as well, as opposed to having more breaking changes in a successive release.
HowardWolosky
added a commit
to HowardWolosky/PowerShellForGitHub
that referenced
this pull request
Aug 11, 2020
The `NoStatus` parameter (and `DefaultNoStatus` config value) were deprecated as part of microsoft#253 when we stopped showing status except for multi-page requests that exceeded some minimum number of pages. At that time, `NoStatus` (and `DefaultNoStatus`) were not removed in order to minimize the churn to the module. However, keeping it in is adding unnecessary complexity to the module as we continue to expand what the module can do. This change removes `NoStatu` (and `DefaultNoStatus`) from the module entirely. The only impact that this may cause is with users who are currently using one (or both) of them. This breaking change impact should be minimal, but its best for the breaking change to be part of the coming release which already has a number of other breaking changes as well, as opposed to having more breaking changes in a successive release.
10 tasks
HowardWolosky
added a commit
to HowardWolosky/PowerShellForGitHub
that referenced
this pull request
Aug 11, 2020
The `NoStatus` parameter (and `DefaultNoStatus` config value) were deprecated as part of microsoft#253 when we stopped showing status except for multi-page requests that exceeded some minimum number of pages. At that time, `NoStatus` (and `DefaultNoStatus`) were not removed in order to minimize the churn to the module. However, keeping it in is adding unnecessary complexity to the module as we continue to expand what the module can do. This change removes `NoStatu` (and `DefaultNoStatus`) from the module entirely. The only impact that this may cause is with users who are currently using one (or both) of them. This breaking change impact should be minimal, but its best for the breaking change to be part of the coming release which already has a number of other breaking changes as well, as opposed to having more breaking changes in a successive release.
HowardWolosky
added a commit
that referenced
this pull request
Aug 12, 2020
The `NoStatus` parameter (and `DefaultNoStatus` config value) were deprecated as part of #253 when we stopped showing status except for multi-page requests that exceeded some minimum number of pages. At that time, `NoStatus` (and `DefaultNoStatus`) were not removed in order to minimize the churn to the module. However, keeping it in is adding unnecessary complexity to the module as we continue to expand what the module can do. This change removes `NoStatus` (and `DefaultNoStatus`) from the module entirely. The only impact that this may cause is with users who are currently using one (or both) of them. This breaking change impact should be minimal, but its best for the breaking change to be part of the coming release which already has a number of other breaking changes as well, as opposed to having more breaking changes in a successive release.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Status for commands was originally added to this module based on my experience with other REST API's where individual commands could easily take 10-20 seconds.
Practical usage has shown that most GitHub requests in reality take under one second. The additional work that PowerShell has to do in order to display progress to the user can easily make the overall command take 4-6 times longer than its actual execution time.
Therefore, status is being ripped out of this module (for the most part).
Invoke-GHRestMethodandInvoke-SendTelemetryEventno longer have bifurcating execution paths based on the value of$NoStatus. Everything runs synchronously now on the command prompt.DefaultNoStatushas been deprecated. Its value will be ignored.NoStatusswitch has not been removed from the module commands in order to avoid a breaking change. It may be removed in a future update.Invoke-GHRestMethod -ExtendedResulthas been updated to include the next page's number and the total number of pages for the REST request.MultiRequestProgressThresholdInvoke-GHRestMethodMultipleResultwill display a ProgressBar to the user tracking the number of remaining requests for the overall execution of the requested command based on this threshold value. It will only display the progress bar if the number of requets needed meets or exceeds this threshold value. This defaults to 10, and can be disabled with a value of 0. Practical usage has shown that thisadds less than a second of additional time to the overall execution of a multi-request command (quite different than the previous status).
Wait-JobWithAnimationhas been removed since it's no longer used.Issues Fixed
Fixes #247
References
Pagination for GitHub API v3 REST API
List Users API which doesn't follow normal pagination guidelines (it uses
sinceinstead ofpageand has nolastlink.Checklist
Changes to the manifest file follow the manifest guidance.Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.Relevant usage examples have been added/updated in USAGE.md.If desired, ensure your name is added to our Contributors list