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

switched some args from options to flags #118

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

NQMVD
Copy link

@NQMVD NQMVD commented Dec 19, 2024

I removed the ArgAction::Set bits for the boolean args to make them flags instead of options (as per claps definition) as that has become the standart and i think it is more convenient too.
I ran the tests before adjusting them in-code and they passed although i'm sure they were not supposed to, might also want to take a look at that.

Tell me if you like the change or prefer it your way.

removed the Action::Set parts for the boolean arguments so they become
flags instead of options per claps definition.
@dhruvkb
Copy link
Collaborator

dhruvkb commented Dec 20, 2024

The main reason I had those as ArgAction::Set was to be able to turn it off. For example, if I have pls aliased to pls --icon=true how could I turn off the icons for one instance? In the current approach, it's possible to invoke pls --icon=false and that disables the icons.

Additionally the use of ArgAction::Set makes these options easier to use with YAML based configuration that's been in the works (#100).

I have been awaiting clap to support negation flags such as --icon/--no-icon that internally maps to a boolean configuration field.

@NQMVD
Copy link
Author

NQMVD commented Dec 20, 2024

Hm, i see. Makes sense and the negation flags would probably be the best solution (except for the YAML config) but the clap issue is 8 years old now and even though it's still kind of active, i don't think that will be implemented any time soon.

I will take a look at it to see if i can create something like that but im still kind of new to rust, so i will start with adding separate negation flags that will override the set flags, and then try to create them automatically.

I haven't looked at the YAML config implementation but there is surely a way to still make it work with negation flags.

added separate negation flags (and changed variable icon to icons)
@dhruvkb
Copy link
Collaborator

dhruvkb commented Dec 20, 2024

I'm very new to Rust as well, so I'm keen to see how other Rust applications approach this and learn from their implementations.

@NQMVD
Copy link
Author

NQMVD commented Dec 24, 2024

I was interested too on how others do it, so i took a look at a few other rust cli programs i saved in my stars over time.

Every program i found with negation flags did it as i did in my last commit, except for one or two that used the when=auto,always,never aproach (bat for example).

I found silicon to have an interesting implementation that allows for both config files (even with multiple languages) and cli args to set config values. It uses structopt, which is outdated and in maintenance mode but there's a migration guide for switching to clap. Might be worth taking a look at, if i don't find anything that's even better without making it too complicated.

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 this pull request may close these issues.

2 participants