lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
401 stars 50 forks source link

Intuitive boolean flags #94

Closed haydenflinner closed 2 years ago

haydenflinner commented 2 years ago

Fixes #68

( I will rebase this shortly to include only the relevant bool change )

haydenflinner commented 2 years ago

How does using this BoolAction as the action change how the boolean arguments appear in the "--help" text on the command-line?


usage: adv-ls.py [-h]  [--follow_symlinks [None]]

optional arguments: -h, --help show this help message and exit

ListDirectoryArgs ['args']: Specs for list-dir.

--follow-symlinks [None], --no-follow-symlinks [None] (default: True)


> If it does change how they appear, then how come the test for the examples haven't picked up on that? Is it because the FieldWrapper is already correcting the metavar to be bool or something like that?

It happens that none of your fields in the tests that test help output are booleans :smile:  at least as far as I can tell.

> If we go the route of auto-generating the "--no-" variants, would we need to also create these arguments using a mutually exclusive group? (I'm really not sure about this though, maybe this is already taken care of by argparse when you pass multiple option_strings for a single argument.)

I'm not sure that it's necessary; it seems that the last flag wins which is fair enough IMO.

python ~/code/python/simpcli3/doc/examples/adv-ls/adv-ls.py --no-follow-symlinks --follow-symlinks --no-follow-symlinks True --follow-symlinks --no-follow-symlinks Received args: ListDirectoryArgs(paths=[], exclude=[], print_format=<PrintFormat.PRETTY: 2>, follow_symlinks=False)

lebrice commented 2 years ago

There should ideally be an example on how to use bools. For now though, having a test that checks the help string is enough for this PR.

And I disagree with the "last option wins". I don't think it should be allowed to contradict oneself by passing multiple arguments that refer to the same thing. Passing the same arg twice though isnt as bad. Those args need to be added in a mutually exclusive group somehow. I'll think about it a bit, but if you have an idea of how that could be done and want to take a crack at it, then be my guest. It could be in another PR, but an issue should be made so I dont forget about it.

Let me know what you think.

JamesDeAntonis commented 2 years ago

Is this PR close to mergeable? I'm dealing with the same problem. To be clear, the implemented behavior would make it so that booleans have "store_true" regardless of the default value?

lebrice commented 2 years ago

Is this PR close to mergeable?

Hey there @JamesDeAntonis ! Not quite. I'd actually be tempted to close this old PR. I don't think a custom action class for the boolean args is necessary. There's probably a much cleaner and easier way to do this.

I'm dealing with the same problem. To be clear, the implemented behavior would make it so that booleans have "store_true" regardless of the default value?

Yes, the idea is that the boolean fields would always have store_true. I was, at some point, successfully convinced that that makes sense, even for arguments like "--no-val" and such.

In the meantime, the best work-around that I've found is to just use a flag (from simple_parsing.helpers.fields), which sets the nargs to 1, instead of "?", so that the arguments are more explicit.

I'll take a closer look at this sometime soon. Also, please feel free, if you'd like to take a crack at it, or if you have some ideas, let me know what you think! :)

JamesDeAntonis commented 2 years ago

Thanks for the response!

sets the nargs to 1

I like this idea, I'll use it