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

Allow accepting an argument by position and by flag #1820

Closed
casey opened this issue Apr 13, 2020 · 21 comments
Closed

Allow accepting an argument by position and by flag #1820

casey opened this issue Apr 13, 2020 · 21 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations S-wont-fix Status: Closed as there is no plan to fix this

Comments

@casey
Copy link
Contributor

casey commented Apr 13, 2020

It would be nice to allow CLIs to a single argument both by position, and by flag.

A possible API for this would be to add Arg::positional(bool), that when passed true makes an arg be accepted by position, even if it already has a long or short flag.

The reason this came up is that wrote a CLI with Clap that takes all of it arguments by flag. After releasing the binary, it's clear that one of the flags is always going to be present, so I'd like to "promote" it to a positional argument, so the flag isn't required. However, I'd like to avoid breaking existing usage, so it would be ideal to make it both a flag and a positional argument.

Allowing seamless promotion from flag to flag + positional would have the nice bonus of encouraging conservative design of CLIs.

Positional arguments are the most scarce, since using more than two or three becomes unwieldy. Short flags are the next most scarce, since there are < 100 possible short flags. Long flags are essentially unlimited. So it would nice to enable the pattern of giving everything long flags initially, and then "upgradiing" commonly used arguments to short flags and positional over time.

@pksunkara
Copy link
Member

I am really hesitant about making this part of clap itself. Clap gives users the tools to do this themselves and I think that is enough for the scope of Clap.

Relevant discussion in #1818

@CreepySkeleton
Copy link
Contributor

I'll go ahead an close this because the described use case is already possible - and not too complicated - with two args and required_unless.

@Dylan-DPC-zz
Copy link

I don't see why this can't be a part of clap. Keeping it open for future exploration

@Dylan-DPC-zz Dylan-DPC-zz reopened this Apr 14, 2020
@CreepySkeleton
Copy link
Contributor

If I get it right, this snipped is exactly what @casey wants:

App::new("app")
    .arg(
        // deprecated optional option
        Arg::with_name("input_opt")
            .hidden(true)
            .long("--input")
            .takes_value(true)
    )
    .arg(
        // new positional arg
        Arg::with_name("input_pos")
            .takes_value(true)
            .required_unless("input_pos")
    )

This is done already, what's left to explore?

@casey
Copy link
Contributor Author

casey commented Apr 14, 2020

Thanks for the workaround, I'll do that for now. I'm not so much thinking of it as deprecating the option for the positional, since options can be more readable, for example in shell scripts.

There are some negatives to using two args. One is that because I'm calling a number of additional methods, those need to be duplicated. Using structopt, but would likely be similar with clap-derive:

  #[structopt(
    long = "input",
    short = "i",
    value_name = "PATH",
    empty_values = false,
    parse(try_from_os_str = InputTarget::try_from_os_str),
    help = INPUT_HELP,
  )]
  input_option: Option<InputTarget>,
  #[structopt(
    value_name = "PATH",
    empty_values = false,
    required_unless = "input-option",
    conflicts_with = "input-option",
    parse(try_from_os_str = InputTarget::try_from_os_str),
    help = INPUT_HELP,
  )]
  input_arg: Option<InputTarget>,

I had to factor out the help message into a constant, INPUT_HELP. I decided to not use hidden, since I think it's slightly more understandable if the argument shows in the help message in the options section, and in the args section at the bottom.

Also, I have required_if constraints on two other arguments on the "input" argument, so I had to modify them to be:

    required_if("input-arg", "-"),
    required_if("input-option", "-"),

And then to actually access the value, since it's now an Option, I'm doing:

    self
      .input_arg
      .as_ref()
      .xor(self.input_option.as_ref())
      .ok_or_else(|| Error::internal("Expected `input_arg` or `input_flag` to be set"))

All in all this isn't too bad, since I'm able to get the CLI that I wanted, but it could be made a little less verbose if it was supported directly.

Edit: Actually, using .xor is more correct, which I missed the first time around.

@pksunkara
Copy link
Member

Also, I have required_if constraints on two other arguments on the "input" argument, so I had to modify them to be:

You can use an arg group for that.

One is that because I'm calling a number of additional methods, those need to be duplicated.

You would be adding just one more condition to check if the argument is present or not. You wouldn't be needing anymore duplication. Please give some example if I am wrong.

I am still not convinced that this is needed to be handled by clap. Let's leave this open and see if more people ask for this.

@pksunkara pksunkara added C: args A-parsing Area: Parser's logic and needs it changed somehow. labels Apr 14, 2020
@casey
Copy link
Contributor Author

casey commented Apr 14, 2020

You would be adding just one more condition to check if the argument is present or not. You wouldn't be needing anymore duplication. Please give some example if I am wrong.

I should have been more clear, what I was referring to was the additional configuration of each arg that needs to be duplicated and now kept in sync, in this case empty_values = false and parse(try_from_os_str = InputTarget::try_from_os_str).

@CreepySkeleton
Copy link
Contributor

I had to factor out the help message into a constant, INPUT_HELP.

This is totally expected when you want to use the same message for different args. For example, consider this clap-unrelated code:

// we want to pass the very same value to multiple fns

// Option 1 - duplicate
func1(&get_val());
func2(&get_val());

// Option 2 - factor out
let val = get_val;
func1(&val);
func2(&val);

clap is no different here.

And then to actually access the value, since it's now an Option, I'm doing:

Yeah, structopt isn't that handy when it comes to requires_if*; this is a known problem. So far, nobody has been able to propose a design that unambiguous and easy to use and considerably easy to implement, but this is a problem with derive, not with clap itself. Your suggestions/help are welcome there!

I was referring to was the additional configuration of each arg that needs to be duplicated and now kept in sync [...]

I believe this is intentional design of clap/structopt (I may be wrong here). Every arg requires you to configure it explicitly so anyone could tell it's properties just from the look of it, no need to look somewhere else. Code is much more read than written after all.

Keeping args in sync might be a problem indeed, but you current proposal is pretty much irrelevant here, no offense.


I'd like to get back to your proposal - positional(bool). In clap, an argument is either positional or not (has long/short options) by definition, they can't be both at the same time. If you want to see it ever implemented, we will need a detailed explanation of the semantic properties backed with unambiguous examples. Thanks for your understanding.

Personally I think that the only real problem here is keeping args in sync, but I also think that requires_if(name) hints developer about dependencies and connections rather well, the state of affairs is "good enough". Everything else here is just a bikeshedding if you ask me.

@pksunkara
Copy link
Member

Can he do flatten to reuse the defined arg?

@CreepySkeleton
Copy link
Contributor

CreepySkeleton commented Apr 14, 2020 via email

@casey
Copy link
Contributor Author

casey commented Apr 15, 2020

I had to factor out the help message into a constant, INPUT_HELP.

This is totally expected when you want to use the same message for different args. For example, consider this clap-unrelated code:

Right, but with access to positional(true), they wouldn't need to be two different args, they could be the same arg, and be configured in a single place.

I was referring to was the additional configuration of each arg that needs to be duplicated and now kept in sync [...]

I believe this is intentional design of clap/structopt (I may be wrong here). Every arg requires you to configure it explicitly so anyone could tell it's properties just from the look of it, no need to look somewhere else. Code is much more read than written after all.

Keeping args in sync might be a problem indeed, but you current proposal is pretty much irrelevant here, no offense.

None taken! I think the proposal is relevant, because if you could do positional(true) the somewhat error prone duplication could be avoided, and done in a single place.

I'd like to get back to your proposal - positional(bool). In clap, an argument is either positional or not (has long/short options) by definition, they can't be both at the same time. If you want to see it ever implemented, we will need a detailed explanation of the semantic properties backed with unambiguous examples. Thanks for your understanding.

Here's a concrete example:

let arg = Arg::with_name("input")
  .long("input")
  .positional(true)
  .takes_value(true);

let app = App::new("app")
.arg(arg);

Then, the following would both be the same, giving the arg input the value foo:

$ app --input foo
$ app foo

@CreepySkeleton
Copy link
Contributor

@casey Sorry for the delay.

The whole point of "different types - different arg" is that we want to keep it simple. In clap, an arg defines one entity, whether it's an option or a positional arg. Your proposal introduces a magical switch that goes against this unspoken policy (I repeat, I might have got it wrong), and what's more, a switch for a thing that does something that is already doable with a combination of existing switches.

I see you called required_unless a workaround, but it's pretty much the intended way to do it. It is intentional - I believe - that when you need two separate entities, you create two separate args and specify their mutual relations via requires*, conflicts*, ArgGroup, whatever. This is a difference between "a god option that does multiple (quite unrelated) things" and "a few smaller options that relate to each other somehow".

My opinion is that your CLI is suffering from "multiple ways to do the same thing" syndrome. You probably don't need it, and if you believe you do, be ready to embrace the complexity it brings in.

@casey
Copy link
Contributor Author

casey commented Apr 18, 2020

@casey Sorry for the delay.

No worries!

The whole point of "different types - different arg" is that we want to keep it simple. In clap, an arg defines one entity, whether it's an option or a positional arg. Your proposal introduces a magical switch that goes against this unspoken policy

I think that part of the issue is that that's not how my mental model of clap works. I think of args as being the same kind of thing, whether by position, by long flag, or by short flag, and that in the same way that the same arg can have both a long flag and a short flag, they should also be able to be positional as well.

@CreepySkeleton
Copy link
Contributor

Our mental models of clap - and perhaps mindsets - are different, indeed. Let me try to explain it to you (and let's hope you're into math 🙏 ).

A developer comes up with a set of command line options (X) at his fancy. clap needs to map these options to a set of Args (Y). What does this mapping look like?

Your mental model is surjective mapping:

image

My mental model is bijective mapping:

image

The reasons I prefer bijection over everything else:

  • It's easy to implement.

  • It's easy to understand. Also, most people comprehend it the same way, which is a priceless property.

  • It's easy to teach. Currently, all I need to explain the difference between "positional arg" and "option" to a newcomer is: "It's positional unless you call Arg::short/long". Dead easy.

    With your proposal in mind, I would have to add "... but if you call Arg::positional, your arg may become both positional and not positional. To understand what this means exactly, please go read that part of documentation".

Anyway, I really don't think this option has any value. The thing you want to do is doable with current clap, the problems with keeping multiple options in sync come from redundancy of your CLI - in my opinion, this is a very edge case that is not worthy of it's own dedicated method. Please don't take the later statement as offense or something, I'm just noticing that the very fact that you are the only one who ever came here with such a request so far (IIRC) it very telling.

If many people come here with similar requests I might become open to it, but until then, I'm totally against the inconsistency the proposed option drags along.

@casey
Copy link
Contributor Author

casey commented Apr 20, 2020

I think it's mostly a matter of perspective: Clap already supports mapping short flags, long flags, and aliases, which could be be considered to be a surjective mapping.

@kbknapp
Copy link
Member

kbknapp commented Apr 29, 2020

@casey I'm sympathetic to your situation, but I'm afraid it will create some additional confusion and complicate the parsing rules for what subjectively is a niche problem set.

I can see where you come from about all arguments being simply "arguments" and the way they're accessed (via short, long, or position) is just an implementation detail. I'd submit that both you and @CreepySkeleton are correct in your assumptions. All arguments are just arguments, and can pretty easily float between the ways they're accessed (short, long, position, etc.) depending on configuration, however @CreepySkeleton is correct in that internally clap handles these as very distinct types, which allows certain optimizations and rule simplification when parsing. The three "types" for clap are:

  • positional (no short or long, essentially just a value)
  • flag (a short or long, but no value)
  • option (a short or long and associated value)

Clap makes the determination as to which type an argument is early on based on the supplied configuration. Having one argument be multiple types could be rather large internal change.

Also, aliases are not related to this discussion as they're just additional shorts/longs and don't change the base type of the argument. I.e. they're not additional arguments from claps point of view (even if it appears so from the users point of view).


To solve your case, I would suggest two things and I know they're not exactly what you're looking for but in the long run I think its a better solution:

  • Set your old option to hidden (i.e. don't show it in the help message)
  • Use required_unless for the new positional argument

Reason being, you are essentially phasing out the old argument. Then on next major version release, you can remove the old option. If you want to go the extra mile, you could first set the option's help message to a deprecation warning, and hide later, or even print a deprecation warning manually if they use the old option.

Essentially you're trying to move your CLI to something more consistent in the future instead of fixing the problem for now but leaving it complicated indefinitely.

@casey
Copy link
Contributor Author

casey commented Apr 30, 2020

That all makes sense, although even considering the maintenance burden of keeping two arguments in sync, I think I probably won't deprecate the option:

  • If all arguments can be passed by option, then the user doesn't need to remember which arguments are passed by position.

  • I personally sometimes find it hard to remember what positional arguments do, and find options easier to remember.

  • For scripts, using options can serve as a form of documentation.

I'm not sure that deprecating the option would benefit the end user.

@kbknapp
Copy link
Member

kbknapp commented Apr 30, 2020

I personally sometimes find it hard to remember what positional arguments do, and find options easier to remember.

100% agree. A good general rule of thumb is to have no more than two positional arguments per command or subcommand for exactly that reason.

@ctrlcctrlv
Copy link

Here's a somewhat minimal (was too lazy to remove some glif2svg options lol) example of how you can hack this together with clap 2.3x despite developers not wanting to add it: https://gist.github.com/ctrlcctrlv/bd0ba2d2a9eb638eec5b5f9dd0ec671e

@ctrlcctrlv
Copy link

Currently, all I need to explain the difference between "positional arg" and "option" to a newcomer is: "It's positional unless you call Arg::short/long". Dead easy.

So don't implement it that way. Implement it as also_long(&str) and also_short(&str). If you call either on a positional argument, or both, then it becomes also optional.

@epage epage added the C-enhancement Category: Raise on the bar on expectations label Dec 8, 2021
@epage
Copy link
Member

epage commented Dec 10, 2021

I think at this point, this is too specialized to have native support in clap but instead we offer the tools to be able to do it.

I have created #3146 to track experiments with multiple arguments mapping to one MatchedArg. Likely it won't work out but I do want to see what it can offer.

I'm going to go ahead and close this. If there are any concerns that are remaining, feel free to speak up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

No branches or pull requests

7 participants