-
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
Add ability to rename a repository: Rename-GitHubRepository #145
Conversation
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.
Thanks so much for doing this, Matt! Very much appreciated!
Thanks as well for writing the extra test and verifying ScriptAnalyzer results as well.
I have some minor feedback for you here to address, but overall, this looks solid. Looking forward to adding it!
Updating the examples to be more uniform (and admittedly slightly less fun) Co-Authored-By: Howard Wolosky <[email protected]>
…is ParamSet, fix style on curly brace, update call to JSON conversion
Howdy, @HowardWolosky -- wanted to be sure that you saw that these are now ready for your re-review. Let me know if anything else catches your eye. Summary: I made the edits you pointed out, added another test, re-ran PSScriptAnalyzer, and re-ran the Pester tests. Yay. Cheers |
Thanks for the update. Will review this PR later this week. |
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.
Matt!
Great update. Thanks so much for handling all of that prior feedback. Just a few tiny further update requests, and then we'll be ready to merge to master.
Yes, I'll get those in this week (back now from vacation) |
…e in certain use cases
Howdy again, @HowardWolosky -- These are now ready for your re-re-review. Let me know what else, if anything. Summary: I made the edits you pointed out, re-ran PSScriptAnalyzer, and re-ran the Pester tests. Yay. Cheers |
Hello, @HowardWolosky Just bumping this to make sure it's still somewhere on the radar. Looking forward to when there is time to return to this PR Cheers |
Apologies for that. Missed your previous bump. Will get to the review this week. Thanks for your patience. |
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.
Really minor remaining feedback. Otherwise, this is good to go.
I'm considering adding that confirmation logic to Remove-GitHubRepository
as well, but it would be a breaking change for users, so I likely wouldn't do it until after we've hit 1.0 (a major version change).
Remove extraneous blank lines, rename vars to remove Hungarian notation Co-Authored-By: Howard Wolosky <[email protected]>
Greets, @HowardWolosky - Feedback incorporated/committed. Let me know if there are any other tidbits. Else, looking forward to the merge! |
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.
🎆 This is great. Thanks so much for the contribution, Matt! 🎆
Looking forward to seeing more contributions from you in the future.
I'll get an update with this change published today or tomorrow.
This has now been published. |
Woo-hoo! Thanks much for working through getting this added. I'm glad to have been able to help a bit. And, hurray for the quick publish, too! See you 'round the community.. |
This adds the ability to easily rename a repository via the new cmdlet
Rename-GitHubRepository
, as suggested in #144 . As we see in the diff, this PR:Running
PSScriptAnalyzer
returns no issue (returned$null
), andPester
test results were unchanged except that there is now one new, successful test.How does this look as an add to this great module?