Skip to content
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

script: Use pflag for command arguments #30

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Dec 12, 2024

Embed the pflag FlagSet into the command creation so we can show decent help for the flags.

The CmdUsage now contains a Flags field of type func(*pflag.FlagSet) (similar to cell.Flagger!) that
allows registering the flags. These flags are then shown in the help summary, e.g.
[-q] [--quiet] for fs.BoolP("quiet", "q", ...) and also expanded in the verbose help.

For example help now looks like:

> help
...
sleep duration [&]
        sleep for a specified duration
stderr [-q] [--count=int] [--quiet] 'pattern'
        find lines in the stderr buffer that match a pattern
stdout [-q] [--count=int] [--quiet] 'pattern'
        find lines in the stdout buffer that match a pattern
stop [msg]
        stop execution of the script
...
> help stdout
stdout [-q] [--count=int] [--quiet] 'pattern'
        find lines in the stdout buffer that match a pattern

        The command succeeds if at least one match (or the exact
        count, if given) is found.
        The -q flag suppresses printing of matches.

        Flags:
              --count int   Exact count of matches
          -q, --quiet       Suppress printing of matches

@joamaki joamaki requested a review from a team as a code owner December 12, 2024 15:39
@joamaki joamaki requested review from dylandreimerink and ovidiutirla and removed request for a team December 12, 2024 15:39
@joamaki joamaki force-pushed the pr/joamaki/script-pflag branch from 3e5a42b to 148a40a Compare December 12, 2024 15:44
@joamaki
Copy link
Contributor Author

joamaki commented Dec 12, 2024

Validated in cilium/cilium with a replace in go.mod and running the pkg/loadbalancer/experimental tests that these changes are backwards compatible, so no changes will be needed to bump to this version.

@joamaki joamaki force-pushed the pr/joamaki/script-pflag branch 4 times, most recently from 5ec99e3 to 55b68a7 Compare December 13, 2024 10:39
Embed the pflag FlagSet into the command creation
so we can show decent help for the flags.

Signed-off-by: Jussi Maki <[email protected]>
@joamaki joamaki force-pushed the pr/joamaki/script-pflag branch from 55b68a7 to 92e3445 Compare December 13, 2024 10:49
@joamaki
Copy link
Contributor Author

joamaki commented Dec 13, 2024

Converted statedb to use this: cilium/statedb#70.

That unfortunately won't be backwards compatible and will need to fix things up when StateDB will be bumped.

Copy link
Contributor

@ovidiutirla ovidiutirla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

if len(args) > 0 && args[0] == "-q" {
quiet = true
args = args[1:]
quiet, err := s.Flags.GetBool("quiet")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be nice to extract these as consts (quiet)

Comment on lines +588 to +589
fs.Bool("readonly", false, "File must not be writable")
fs.Bool("exec", false, "File must not be executable")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: also these flags too, maybe? I was thinking to have a way to be able to re-use these flags that have high chance to be re-used across cmds.

@@ -637,26 +643,20 @@ func Grep() Cmd {
})
}

const matchUsage = "[-count=N] [-q] 'pattern'"
func matchFlags(fs *pflag.FlagSet) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nicely reused

@joamaki joamaki merged commit 605c141 into main Dec 13, 2024
1 check passed
@joamaki joamaki deleted the pr/joamaki/script-pflag branch December 13, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants