Conversation
|
@mislav I've implemented the |
|
Hey @mislav just wanted to follow up a little on this. Thank you :) |
vilmibm
left a comment
There was a problem hiding this comment.
This is a good start!
The tests make sense; it looks like you have them set up properly.
I was confused by the difference in nomenclature between allow and enable flags -- if they are both toggles that accept true or false, they should probably be named similarly.
Some other stuff:
- instead of
-n, support-Ras other commands do - avoid printing a success message if nothing was changed
One final consideration at this point is around renaming. We've added gh repo rename, so it's not strictly necessary that this command can rename; but it might feel weird to not support it. For now it's okay to punt on renaming in this command and we can worry about it later.
|
@vilmibm thank you for the feedback. I think we can go with |
|
@g14a sounds good; as far as the nothing being changed, i just noticed that even with no flags passed to the command ( |
|
@vilmibm made the required changes. |
|
Hey @vilmibm just wanted to follow up regarding the changes.. thank you :) |
|
@g14a I'm looking over this now, will get back to you soon! Thanks for your work and your patience 🙇 |
There was a problem hiding this comment.
Hi @g14a, instead of requesting changes, I've pushed some simplifications to the branch.
- The most important change is that the
PATCHAPI request sent to update the repository will no longer contain all the parameters with their default values. Only explicitly edited fields are ever sent. This is accomplished by using pointer fields on the EditRepositoryInput struct and marking them asomitemptyfor JSON serialization. This avoids race conditions in repository updates, e.g. two different update operations that concurrently write to different fields on the same repository shouldn't reset each other's changes. - To make defining flags with pointer receivers easier, I've added helper functions for this to the
cmdutilpackage. - A flag that wasn't passed at all will have its pointer value at nil. We can thus avoid excessive checks to
cmd.Flags().Changed(). - I've dropped the check that raises an
at least one merge option must be trueerror since it didn't make sense in the context of this command. - I've also dropped the check for the validity of the
--nameflag. We let the server decide about the validity of values passed. - For now, I've removed the flag for changing the privacy/visibility of the repository. We have to first decide on an API for explicitly changing the visibility of the repository, and I would suggest avoiding boolean flags like
--private. How about:--visibility={public,private,internal}? - For now, I've removed the flag to archive the repository since we have the
gh repo archivecommand now.
Could you go over the changes to see if they make sense? Thank you! 🙇
samcoe
left a comment
There was a problem hiding this comment.
This looks really clean with the new flag utility functions!
|
@mislav damn! The new flag util functions look amazing and evreything looks much much cleaner! This feature was a roller coaster for me. Thank you :) Let's ship this to the moon 🚀🚀 |
Co-authored-by: Mislav Marohnić <mislav@github.com>
|
Pushed updates:
|
This PR adds a
repo editcommand to update settings of a repository.Closes #995