BUG: Useing altsrc when flag has multiple names

[dixonwille]

In this code, you are never using the eachName's functions variable name. This caught my attention then realized that f.Name could be equal to user,u which you will never be set in context. So then I realized that the eachName should wrap everything inside the if f.set. then changing all f.Name inside to name. Also seems like f.GenericFlag.Name should also be renamed to name.

[jszwedko]

It does feel like name should be used there, but I'll let @ChrisPRobinson comment in case I'm missing something. Thanks for bringing this up @dixonwille.

[ChrisPRobinson]

I agree with @jszwedko, it seems like a bug, this seems like we should use name instead of f.Name.

I don't think I really had much testing around the scenario where there are multiple of one flag. Are you seeing bad behavior when the short flag and long flag are used in combination @dixonwille ?

[dixonwille]

I am but replacing that single variable didn't seem to work. context.IsSet(f.Name) stops it from ever getting to that loop. f.Name is user,u for example and since that is not a flag (user is a flag and u is a flag) it skips over the whole computation. @ChrisPRobinson

[ChrisPRobinson]

@dixonwille perhaps you can give a pointer to your actual code and tests? I'm not exactly sure of the problem. My approach would be to add the test scenario I want to work and evaluate its behavior, sorry if I'm repeating myself from above. I don't have a ton of time to attempt this work myself right now but could review your code if that helps?

[dixonwille]

Code is on internal Enterprise Github. Here is the snippet though.

altsrc.NewStringFlag(cli.StringFlag{
        Name:  "user",
        Usage: "The SQL `USERNAME`",
}),

I would like to make the name user,u. I am also busy and understand. I'll see if I can get some more info for you or try and fix it myself and make PR. Not sure when I will have time but I will try my best.

[ChrisPRobinson]

So to clarify what you want to do is something like this?
https://github.com/urfave/cli#alternate-names

Based on the example above you would need to change it to this?

altsrc.NewStringFlag(cli.StringFlag{
        Name:  "user, u",
        Usage: "The SQL USERNAME",
})

Then when you put a command line like this it doesn't work?

cmd do-something --u chris --user dixon

[dixonwille]

Well passing it through as flags it works. It's loading from the file into the flags that is not working. If I add ,u (the only line that I change) then the flag is never set from file. But if I remove it and just have user then loading from a file works great!

[ChrisPRobinson]

In the yaml file do you want to be able to value by both names, u and user?

This may not compile but can you tweak the function below to something like this?

func (f *StringFlag) ApplyInputSourceValue(context *cli.Context, isc InputSourceContext) error {
    if f.set != nil {
        if !(context.IsSet(f.Name) || isEnvVarSet(f.EnvVar)) {
            if value != "" {
                eachName(f.Name, func(name string) {
                                    value, err := isc.String(name)
                        if err != nil {
                        return err
                          }
                       f.set.Set(f.Name, value)
                })
            }
        }
    }
    return nil
}

So I think what its doing for yaml anyway is its trying to do a lookup for "user, u" and not user. Then it doesn't find it and no value is returned.

[dixonwille]

That is what I would expect to happen anyways. But yes that is what I think is going on. When I have time I will try a few things but starting there.

[coilysiren]

Hiya! There's a known issue with the current implementation of the CLI, where altsrc is generally clunky and poorly documented. My current idea is that I'm going to move all of the altsrc functionality into the main package in some way, and that's how all of the "altsrc is weird and different???" issues people are having will get resolved. I'm tracking that work here => #833