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

Don't raise an error when a choose-one flag is specified repeatedly #553

Closed
sarahgerweck opened this issue Jul 13, 2017 · 8 comments
Closed
Labels
enhancement An enhancement to the functionality of the software. question An issue that is lacking clarity on one or more points.

Comments

@sarahgerweck
Copy link

There is currently no way to set custom defaults for Ripgrep. The recommended practice is to set an alias instead. However, this is somewhat thwarted by the fact that Ripgrep is too aggressive in validating that you never repeat yourself. This also has other consequences.

E.g., if you prefer smart case matching, you can make an alias like this:

alias rg='rg -S'

This mostly works, but it means that I now get an error if I ever execute any commands that include -S in them. Ripgrep give the error below:

error: The argument '--smart-case' was provided more than once, but cannot be used multiple times

This is undesirable behavior both when writing scripts and when trying to use aliases as defaults. Both scripts and aliases face the same problem: building up a complex command line by nesting contexts. This rule means that this process needs to be globally aware of all the outer contexts.

I think it is much more common and more user friendly that commands like this use the last one specified for flags that are mutually exclusive, and don't raise errors if you specify the same flag twice. Otherwise, trying to build nontrivial command lines from variables or aliases is somewhat a nightmare.

Fortunately, it at least allows rg -S -i. I think this is a good example of why raising an error for rg -S -S is undesired behavior. The rg -S -i case is more overspecified than the rg -S -S case. Ultimately, the current rule doesn't actually protect you from any undesired behavior, but it does prevent useful use cases.

@BurntSushi
Copy link
Owner

Right, agreed. My feeling is that this should get lumped into #196 (where I also left a comment explaining some things just now), although I can appreciate that these are really two orthogonal issues. It just so happens that persistent config is blocked on it. But we can make your situation better without needing to finish persistent config, and this issue is thorny enough that I think giving it its own ticket is a good idea.

As a possible work-around until this is fixed properly, if you prefix your command with \ (e.g., \rg -S pattern), then your shell should ignore any aliases you have set up and invoke the command directly.

This is undesirable behavior both when writing scripts and when trying to use aliases as defaults. Both scripts and aliases face the same problem: building up a complex command line by nesting contexts. This rule means that this process needs to be globally aware of all the outer contexts.

I think you might find that you have this problem regardless, although I can agree that the particular version of the problem you have might be the most annoying version of it. In particular, if you write a script that assumes your default config, then it might fail when the default config isn't present (on a different machine) or if it changes over times as your preferences evolve. My "answer" to that is to avoid using custom defaults inside scripts.

@BurntSushi BurntSushi added enhancement An enhancement to the functionality of the software. question An issue that is lacking clarity on one or more points. labels Jul 13, 2017
@sarahgerweck
Copy link
Author

sarahgerweck commented Jul 13, 2017

Yeah, I agree that custom settings can create problems with scripts & shared commands in general. The point I was making was that if I wanted to send a coworker a command I use, I could just add the -S that is part of my alias—but then I'll get complaints if I run it. You're right that I could work around this by temporarily unaliasing it or escaping it: it's just a matter of convenience.

Neither this nor #196 is an earth-shattering issue. Ripgrep is a great tool and a nice performance and .gitignore-compliance improvement over Silver Searcher: this is just a quality-of-life issue.

@okdana
Copy link
Contributor

okdana commented Jul 13, 2017

btw, i believe clap #976 is the specific ticket that covers this limitation it has. I don't think i like the suggested solution (it should be the default behaviour, you shouldn't have to write a bunch of code for every single option) but that's where the immediate fix would come from i think.

tbh, these sorts of problems are common to almost every library that tries to provide a more 'modern' approach to argument-handling than getopt(3)/getopt_long(3). Python's argparse (and optparse before it), Swift's Commander, PHP's Symfony Console and Zend Console, &c. — they all have really frustrating limitations when it comes to dealing with options that appear multiple times or whose position in the argument list is supposed to be meaningful.

clap is actually really functionally advanced compared to all of those, but you still run up against some fundamental issues that would be very simple to deal with if the library would just give you back the list of arguments so you could handle them yourself with a for loop.

Given all of what clap already does, though, this particular issue doesn't seem like it'd be too difficult to solve. Probably just a matter of time and priorities.

BurntSushi added a commit that referenced this issue Feb 4, 2018
This commit builds on the previous argv refactor by being more principled
about how we declared our flags. In particular, we now require that every
clap argument is one of three things: a positional argument, a switch or
a flag that accepts exactly one value. The latter two are always permitted
to be repeated, and we modify the code that consumes a clap::ArgMatches to
always use the *last* value of an argument. (clap by default always uses
the first value of argument, if it has been repeated and is accessed via
one of the singleton accessors.)

Fixes #553
@kbknapp
Copy link
Contributor

kbknapp commented Feb 5, 2018

clap-rs/clap#976 has finally been fixed, implemented and released (v2.29.3). I know there is a work around for this, but just FYI.

@BurntSushi
Copy link
Owner

@kbknapp Awesome! Thanks so much! I will switch over to that and get rid of my work-around. :-)

@BurntSushi
Copy link
Owner

@kbknapp Hmm it looks like I can't just use AllArgsOverrideSelf because it causes flags that are supposed to be able to be specified multiple times to not work correctly. For example, if I enabled AppSettings::AllArgsOverrideSelf, and if I run

$ rg 'fn run' src/ --colors 'match:fg:blue' --colors 'path:fg:blue'

then only --colors 'path:fg:blue' will be applied and --colors 'match:fg:blue' will be ignored.

@kbknapp
Copy link
Contributor

kbknapp commented Feb 6, 2018

🤦‍♂️ I fixed that for the switch case and forgot to fix it for the option case....standby

@kbknapp
Copy link
Contributor

kbknapp commented Feb 6, 2018

Fixed in clap-rs/clap#1166

michaliskambi added a commit to michaliskambi/elisp that referenced this issue Jul 25, 2018
Mostly to get this bugfix: BurntSushi/ripgrep#553 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement to the functionality of the software. question An issue that is lacking clarity on one or more points.
Projects
None yet
Development

No branches or pull requests

4 participants