-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: Command.SetVersionFunc #2313
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
base: main
Are you sure you want to change the base?
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.
Some minor remarks
func TestVersionFuncExecuted(t *testing.T) { | ||
rootCmd := &Command{Use: "root", Run: emptyRun, Version: "v2.3.4"} | ||
rootCmd.SetVersionFunc(func(cmd *Command) error { | ||
_, err := fmt.Fprint(cmd.OutOrStdout(), "custom version function: "+rootCmd.Version) | ||
return err | ||
}) | ||
|
||
output, err := executeCommandWithContext(context.Background(), rootCmd, "-v") | ||
if err != nil { | ||
t.Errorf("Unexpected error: %v", err) | ||
} | ||
|
||
checkStringContains(t, output, "custom version function: v2.3.4") | ||
} |
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.
Could you test when versionFunc is only available on the parent?
Thanks
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 actually not sure how that should work, as the version
flag is only registered to the root command AFAIK. Maybe someone that knows the codebase better can chime in.
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 don't know, but the parent.parent.parent thing makes me think there is something to double check
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.
@caarlos0 unless I'm wrong this wasn't addressed yet.
I would like to see a test to cover this.
2052db2
to
9a87291
Compare
Currently, we can use `Command.Version` and `Command.SetVersionTemplate`, which generally works fine. I would like to propose a `Command.SetVersionFunc`, similar to `command.SetHelpFunc`. The rationale behind it is that the caller can style the output conditionally, e.g.: ``` $ pgr -v styled version output $ pgr -v | cat plain-text version output (output not a tty) ``` Right now, the way around it is to, before executing the root command, check if output is a terminal, detect color profile, build and set a styled template. This works, but can interfere with content being piped in/out (e.g. if you want to detect terminal background color and other terminal features). Signed-off-by: Carlos Alexandro Becker <[email protected]>
9a87291
to
babb335
Compare
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.
Pull Request Overview
This PR adds a new SetVersionFunc
method to the Command struct, allowing users to define custom version output functions that can conditionally style output based on terminal properties.
- Added
Command.SetVersionFunc
method to enable custom version output functions - Added version function field to Command struct and corresponding getter method
- Added test coverage for the new version function functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
command.go | Implements the core SetVersionFunc functionality with struct field, setter method, and execution logic |
command_test.go | Adds test case to verify custom version function execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Currently, we can use
Command.Version
andCommand.SetVersionTemplate
, which generally works fine.I would like to propose a
Command.SetVersionFunc
, similar tocommand.SetHelpFunc
.The rationale behind it is that the caller can style the output conditionally, e.g.:
Right now, the way around it is to, before executing the root command, check if output is a terminal, detect color profile, build and set a styled template.
This works, but can interfere with content being piped in/out (e.g. if you want to detect terminal background color and other terminal features).