Add confirmation prompts and examples for Remove- functions#174
Conversation
HowardWolosky
left a comment
There was a problem hiding this comment.
Thanks so much for this, @themilanfan!
Really minor feedback to address, and then I'll be happy to merge this in. Thanks again, and welcome to the project!
HowardWolosky
left a comment
There was a problem hiding this comment.
Actually, just realized that you didn't add the necessary ShouldProcess commands to have any impact here.
See
PowerShellForGitHub/GitHubProjects.ps1
Line 496 in d279f1c
You probably need to update a bunch of tests too in order for them to pass (adding -Confirm:$false to those calls), because once this is done, all of the existing tests that make use of Remove-* commands will fail because they'll hang waiting for user confirmation.
…pt for confirmation Also re-addded needed whitespaces
Added line-space in example
|
@HowardWolosky thank you for the feedback, I've just committed the changes! |
giuseppecampanelli
left a comment
There was a problem hiding this comment.
Completed in new commits
giuseppecampanelli
left a comment
There was a problem hiding this comment.
Completed review in commits
|
@HowardWolosky I'm not sure what's happening with the check, I reviewed all the changes you requested and thought it would resume |
|
@themilanfan - Thanks for the update. Will review the changes this weekend. As for the checks, ignore them for now. I've been running into issues with the tests properly running in PR CI builds, so I still need to verify them locally before merging. I'll get to fixing the PR CI build at some point. |
HowardWolosky
left a comment
There was a problem hiding this comment.
Overall, this looks like a great update, thanks!
One minor documentation suggestion in this review.
Additionally, more tests have been added since you authored this PR. Would you be willing to refresh this PR with the latest changes from master, and incorporate any remaining -Confirm:$false parameters into any new instances of Remove-* methods being called in the tests?
Thanks so much!
Co-authored-by: Howard Wolosky <HowardWolosky@users.noreply.github.com>
|
Hi @HowardWolosky, I've added the remaining -Confirm:$false as well as updated the documentation and fixed another small issue in GitHubLabels.tests.ps1 as well. I will also wait for this to merge so that for issue #176 I don't miss any Confirm:$false there. |
|
Removed them 😄 |
|
It looks like some conflicts came in as a result of some of the recent commits as I've been trying to get the CI to run clean on all platforms. Can you please address the conflicts? Then I'll give it a run through the CI and get it committed....Thanks! |
|
Resolved conflicts 😄 |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
HowardWolosky
left a comment
There was a problem hiding this comment.
This is ready to go. Thanks so much!
Fixes #171