- Notifications
You must be signed in to change notification settings - Fork928
chore:version sub command remove--version and-v flag#2090
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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 for picking this one up! 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
cli/root.go Outdated
| func versionCmd() *cobra.Command { | ||
| return &cobra.Command{ | ||
| Use: "version", | ||
| Short: "Version for coder", |
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.
Suggestion: I think this reads a bit better, but up to you to include or not.
| Short:"Version forcoder", | |
| Short:"Showcoder version", |
cli/root.go Outdated
| cmd.Flags().BoolP("placeholder", "v", false, "This flag is a placeholder to make the cobra version flag work appropriately") | ||
| _ = cmd.Flags().MarkHidden("placeholder") |
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.
Cobra doesn't add this flag ifVersion isn't specified in the root. I'd propose that being a cleaner alternative.
Having--version andcoder version seems unnecessary to me. I'd prefer to havecoder version.
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’m fine with only havingversion but having both is consistent withhelp (or do I misremember?) and I think covers a common use case. I.e. there’s a 50/50 chance someone will blindly do either--version orversion instead of checking help 😄.
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 like how Go just doesgo version. I haven't found it to be confusing tbh
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.
Correct cobra does not. However if we want to support--version, we would need a persistent pre-run function to support the flag. Cobra has some first class support for--version that works even if there are extra args, eg:coder --version workspaces.
I think it is reasonable to use their first class support for this reason.
As for--version orcoder version. I don't see anything wrong with supporting both. It's nice as a user to do w/e is more familiar to you.
Maybecoder version can report more info like the version of the deployment you are logged into.
Examples:
git version&git --versiondocker --versionis less verbose thandocker versiondocker-compose --versionis less verbose thandocker-compose version
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.
It's fine to support both, I don't really understand why we need to though. Seems like supportingcoder version is fine, and supporting both seems to not really satisfy a need, but adds an additional user-contract for us to test.
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'll drop the flag for now then and just docoder version
version sub command and reserve-v flag on root.version sub command remove--version and-v flagUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
What this does (#1543)
Removes
coder -vandcoder --version.Now do
coder versionto see coder version.