akamensky / argparse

Argparse for golang. Just because `flag` sucks
MIT License
604 stars 62 forks source link

Arg can return the result pointer #84

Closed vsachs closed 3 years ago

vsachs commented 3 years ago

This allows for more dynamic flag/arg handling, such as scenarios where:

~/git/github.com/vsachs/argparse| go test
PASS
ok      github.com/akamensky/argparse   0.015s
vsachs commented 3 years ago

Sorry about the commit history, I don't understand why it looks like that. Hopefully you can squash or fixup those?

akamensky commented 3 years ago

@vsachs Thank you for the contribution. I do appreciate the time you took to work on this and I am not against it, but just for the sake of understanding the consequences of these changes, can you please elaborate on the use cases (preferably with example code)?

vsachs commented 3 years ago

@akamensky Happy to oblige

The scenario is that we already have a CLI utility with roughly this argument profile: utility cmdA utility cmdB --flagA --flagB --flagC utility <cmdC|cmdD|cmdE|cmdF|cmdG> --flagA --flagB --flagC --flagD --flagE

We want to add flags A-E to cmds C-G but we don't want to have to track 5 * 5 named local variables. With the current code, this means we have to create a map[string]interface{} keyed off the flag names and store the returned flag pointers for each command. This is just a duplication of the data already stored privately inside the Command args field.

If we have the GetResult() functionality we can drastically simplify and do something nifty like this:

func parseArgs() {
    parser := argparse.NewParser("util", "does stuff")
    parser.NewCommand("cmdC", "c command")
    parser.NewCommand("cmdB", "c command")
    for _, name := range(dynamicCommandList) {
        cmd := parser.NewCommand(name, name + " command")
        cmd.String("a", "flagA", nil)
    }
    for _, cmd := range(parser.GetCommands()) {
        if cmd.Happened() {
            for _, arg := cmd.GetArgs() {
                if arg.GetResult() != nil {
                switch val := (*arg.GetResult()).(type) {
                case string:
                    configStuff[arg.GetLname()] = val
                case int:
                    configStuff[arg.GetLname()] = fmt.Sprintf("%d", val)
                }
            }
        }
    }
}

There are other ways to approach this, such as returning the dereferenced value instead of the pointer, but for a somewhat niche use case it seemed better to leave the library with greater flexibility rather than binding it to a tighter API.

akamensky commented 3 years ago

Got it. I think this is fine. I would personally go with another approach, but it's the matter of taste. Can you then please document this functionality in README file and in code (so IDEs can pick up description).

vsachs commented 3 years ago

Got it. I think this is fine. I would personally go with another approach, but it's the matter of taste. Can you then please document this functionality in README file and in code (so IDEs can pick up description).

Will try to do that today, been a scramble this week

vsachs commented 3 years ago

@akamensky How would you like me to document it in the code? I only use emacs myself so I'm not familiar with how IDEs are doing this kind of thing.

None of the other similar functions in argument.go have docstrings (GetOpts, GetSname, GetLname)

akamensky commented 3 years ago

@vsachs Normally there is a godoc type comment that explains public method, their arguments and return values. It is used by godoc tool to generate docs. I.e. like this:

// exampleFunc does some example work. It accepts one string argument. If error happened
// err will be not nil and integer will be set to 0.
func exampleFunc(arg string) (int, error) {
    ...
}