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

Unable to fully capture the rest of arguments and pass it through to another command #253

Closed
mitar opened this issue Dec 21, 2021 · 17 comments · Fixed by #262
Closed

Unable to fully capture the rest of arguments and pass it through to another command #253

mitar opened this issue Dec 21, 2021 · 17 comments · Fixed by #262

Comments

@mitar
Copy link
Contributor

mitar commented Dec 21, 2021

So I am trying to implement exec pattern, where I have a subtool I want to execute and pass all other arguments to it. Trying passthrough:"" does not fully work with --help flag. I have something like:

type Commands struct {
	Exec ExecCommand `cmd:"" help:"Run the sub-tool.`
}

type ExecCommand struct {
        Arg []string `arg:"" optional:"" passthrough:"" help:"Arguments passed on to the sub-tool."`
}

Inside ExecCommand's Run I do: subtool.Main(append([]string{"subtool"}, c.Arg...)) to pass all args to the subtool's CLI parsing.

And this does not work. So if I do:

tool exec foobar

it works well, and sub-tool gets foobar as an argument.

But:

tool exec --help

returns help of tool and not of the sub-tool.

A workaround is:

tool exec -- --help

which does work. But how can I make it work without --? I think this might be related to #216 and in general that I would prefer that global flags do not get inherited by the subcommands. So in general tool --help subcommand should work and tool subcommand --help should not, unless subcommand explicitly defines --help flag.

I tried to move passthrough:"" to Exec itself, but I got panic passthrough only makes sense for positional arguments.

@alecthomas
Copy link
Owner

My guess is --help is triggering somehow because it's using BeforeApply(), but this looks like a bug to me in that it should work.

@mitar mitar changed the title Unable to full capture the rest of arguments and pass it through to another command Unable to fully capture the rest of arguments and pass it through to another command Dec 21, 2021
@mitar
Copy link
Contributor Author

mitar commented Dec 21, 2021

I think the issue is in order of processing tokens, FlagToken is before PositionalArgumentToken, where endParsing is called. So --help is then parsed first as FlagToken, and then PositionalArgumentToken comes next. On the other hand, -- is processed early so this is why it then works.

@mitar
Copy link
Contributor Author

mitar commented Dec 21, 2021

I think that allowing passthrough on a command would be the solution here. So once exec is parsed, it calls endParsing. Which somehow makes sense to me. exec is the keyword which triggers special parsing of the rest, not some positional argument later on. exec is in this case more like typed --. :-)

@alecthomas
Copy link
Owner

I think there are two use cases and both seem valid:

  1. (the current one) where exec [<exec-flags] <remainder>... functions - this allows exec to have its own flags.
  2. (not supported) the one you're referring to: exec <remainder> where exec cannot have its own flags

@alecthomas
Copy link
Owner

alecthomas commented Dec 21, 2021

Allowing passthrough on a command seems reasonable to me, though it will need some extra sanity checks to make sure the command only has a single Args []string field.

@mitar
Copy link
Contributor Author

mitar commented Dec 21, 2021

After more thought, I realized two things.

First, the problem is larger than I thought. Running tool exec --invalid does not call the subtool with --invalid, but fails at parsing already.

The other is that I think the core of the issue is in flags being inherited from the parent. This is why it is surprising that --help exists there. I think the difference between exec [<exec-flags] <remainder>... and exec <remainder> should be configured by simply: are exec-flags defined on the command or not.

So I think the solution here is to do:

  • If there are passthrough enabled positional arguments, then parsing should never fail but move anything which failed parsing to positional arguments.
  • There should be an option noInheritance:"" or something, which would prevent inheriting parent flags, or in other words disabling interspersed for that sub-command. I find this better than kong.AllowInterspersed(false) (despite being more verbose as somebody might have to include that on all sub-commands) as it allows one to combine both approaches to parsing. For example, in the example above, I could disable inheritance only for exec sub-command (to get rid of --help there), but keep it for other sub-commands.

Bonus question: how should tool exec -- something be parsed into Arg? As ["--", "something"] or as ["something"]? I am slightly leaning towards the former (given that -- should not have to be used at all there if no flags have been defined, only Arg). But what if there would be flags defined for ExecCommand? Then -- should be available at exec level and not passed on.

@alecthomas
Copy link
Owner

alecthomas commented Dec 24, 2021

The problem with allowing only flags under a command to be parsed, then passing anything else on, is that if you add more flags to the command in the future you may break previously working commands.

For example, say the first version of the command had one flag --flag, while --second is passed through the args:

cmd --flag --second

If you subsequently add a --second flag to cmd itself, that flag will now no longer be passed through.

I feel like the cleanest option is your initial suggestion of supporting passthrough on commands, and the semantics would be that no flags are allowed, only a single repeated arg that all subsequent tokens would be captured into.

eg.

Cmd struct {
  Args []string `arg:""` // <- required and only field allowed
} `cmd:"" passthrough:""`

@mitar
Copy link
Contributor Author

mitar commented Dec 25, 2021

The problem with allowing only flags under a command to be parsed, then passing anything else on, is that if you add more flags to the command in the future you may break previously working commands.

Sure, but doesn't this hold also if I pass everything on after cmd, but then later on decide that cmd needs --flag and I add it (removing passthrough:"" on the command). Then --flag will not be passed through anymore.

What I am trying to say is that the mechanism how we describe in the configuration of CLI parsing to pass things on to a sub-tool does not prevent breakage by itself. As a developer managing this configuration I can always introduce changes which break previously working commands. I think this can be addressed only by documenting properties and gotchas of the configuration so that developer can choose the right modification and know what are the consequences. E.g., if we add passthrough:"" on commands we have to document that if you remove this at a later stage, you will be breaking potentially something which worked before. If we provide a way that unparsed flags are passed through, then we have to explain that adding a new parsed flag will be breaking potentially something which worked before. It is true that if you have flag inheritance enabled, then with the later approach you might be adding a new flag without realizing it (because you are adding it to the parent).

But if you prefer to go this route, I would add that I find configuration which forces the structure a bit ugly (i.e., a bit forced and not nicely orthogonal). Why do I even need nested struct at that point? Like, why not have:

Cmd []string `cmd:"" passthrough:""`

@alecthomas
Copy link
Owner

Sure, but doesn't this hold also if I pass everything on after cmd, but then later on decide that cmd needs --flag and I add it (removing passthrough:"" on the command). Then --flag will not be passed through anymore.

Yes but that's explicit, whereas the other behaviour is indirectly implicit and not at all obvious until it bites you.

But if you prefer to go this route, I would add that I find configuration which forces the structure a bit ugly (i.e., a bit forced and not nicely orthogonal). Why do I even need nested struct at that point? Like, why not have:

Because this is inconsistent with the rest of Kong and additionally it wouldn't directly work with Run().

@mitar
Copy link
Contributor Author

mitar commented Dec 25, 2021

Because this is inconsistent with the rest of Kong and additionally it wouldn't directly work with Run().

Hm, what about:

type Args []string
Cmd Args `cmd:"" passthrough:""`

That works with Run() (on custom type Args)?

@mitar
Copy link
Contributor Author

mitar commented Jan 3, 2022

(FYI, it is your package, so feel free to just make a decision here. I am suggesting alternatives because I am trying to see if we can find a better design here, but if you feel we already reached it, just say so.)

@alecthomas
Copy link
Owner

I've already stated my position after quite a bit of back and forth, so I saw no need to continue discussing it further. It is:

Cmd struct {
  Args []string `arg:""` // <- required and only field allowed
} `cmd:"" passthrough:""`

But regarding also allowing flags, I think it would be sufficient to have an explicit whitelist in this situation, with an error if a flags is added without also being in the whitelist. eg.

Cmd struct {
  Flag1 string
  Flag2 bool // Kong will error on this flag
  Args []string `arg:""` // <- required and only field allowed
} `cmd:"" passthrough:"" allow-flags:"flag1"`

@mitar
Copy link
Contributor Author

mitar commented Jan 4, 2022

Sounds good. I will see if I can get to this any time soon and make a PR.

I do not get why we would need allow-flags so I will leave that out for now (you can always just move passthrough to an argument if you want additional flags).

mitar added a commit to mitar/kong that referenced this issue Jan 4, 2022
@mitar
Copy link
Contributor Author

mitar commented Jan 4, 2022

I made #262.

@alecthomas
Copy link
Owner

I do not get why we would need allow-flags so I will leave that out for now (you can always just move passthrough to an argument if you want additional flags).

Because it's explicit, and because it will work correctly for flags on parent commands. But yeah we can worry about this later. Your PR is a great start, thank you.

@mitar
Copy link
Contributor Author

mitar commented Jan 5, 2022

and because it will work correctly for flags on parent commands

I see. Thanks for the explanation.

mitar added a commit to mitar/kong that referenced this issue Jan 5, 2022
alecthomas pushed a commit that referenced this issue Jan 5, 2022
@alecthomas
Copy link
Owner

Great stuff, thanks again. Let me tag a new release with this in it.

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 a pull request may close this issue.

2 participants