Skip to content

Fix boolean options#500

Merged
inclement merged 4 commits into
kivy:masterfrom
denys-duchier:fix-boolean-options
Nov 28, 2015
Merged

Fix boolean options#500
inclement merged 4 commits into
kivy:masterfrom
denys-duchier:fix-boolean-options

Conversation

@denys-duchier

Copy link
Copy Markdown
Contributor

type=bool is not the way to do boolean options with argparser. I added add_boolean_option and used it to automatically define both --foo and --no-foo options.

@inclement

Copy link
Copy Markdown
Member

This seems quite complicated - would doing it like e.g. http://stackoverflow.com/a/15008806/2469283 be better? It seems to avoid the need for string mangling outside of argparser.

@denys-duchier

Copy link
Copy Markdown
Contributor Author

I did it like that at first, but then I realized that the automatically generated help was hard to read. so I added groups for each pair of boolean options. I didn't go the t_or_f way because that's not the canonical way to express boolean options. Also I wanted to preserve existing options such as --allow_download, but also provide the more standard --allow-download. I don't find my code to be complicated at all (but of course YMMV), and it makes declaring boolean options very easy.

@inclement

Copy link
Copy Markdown
Member

The muckery to allow both _ and - was probably the main thing that seemed messy to me. How would you feel about adding just the - versions and not supporting the underscores - I only did it that way in the first place because I wasn't familiar with this convention (I now understand that dashes do seem normal?). At this point, I think it's fine to do a small breaking change like this.

Could you also pep8-style the code, there are just a few things like missing spaces after commas?

@denys-duchier

Copy link
Copy Markdown
Contributor Author

if you are ok with the breaking change, then I am even happier. I'll update the PR accordingly.

@denys-duchier

Copy link
Copy Markdown
Contributor Author

I made pep8 shut up, but there were lots of issues I wasn't responsible for :-)

@inclement

Copy link
Copy Markdown
Member

No problem, it was just pep8 of the new code I'm worried about, the rest is of course not your problem (actually I have to merge a tshirtman commit to fix them).

@denys-duchier

Copy link
Copy Markdown
Contributor Author

they are all fixed in my PR (for toolchain.py)

@inclement

Copy link
Copy Markdown
Member

Oh, in that case thanks even more!

@inclement

Copy link
Copy Markdown
Member

I had a query on #kivy-dev, but I guess I missed you. It was:

The boolean option one seems fine, but how would you feel about stripping it to be simpler? There isn't actually really a need to support multiple args doing the same thing (presumably this stems originally from supporting the underscores?), and I also think that manually messing with the -- prefix is unnecessary.

If that sounds reasonable to you, I can make those changes myself on merging, if you don't want to bother updating the PR.

@denys-duchier

Copy link
Copy Markdown
Contributor Author

ah yes, I went to sleep. multiple names are still needed if you want to provide both long and short option names. I think argparse already does replace - in the option name with _ to get the default dest, so that could be taken out. The case for no versions computed from names could be simplified if you are willing to live with --foo_bar (which someone could very well choose to do) yielding --no-foo_bar.

tell me what you think. right now I have to go teach a class... I'm late :-/

@inclement inclement merged commit b75d630 into kivy:master Nov 28, 2015
@inclement

Copy link
Copy Markdown
Member

I've merged this, thanks again for it.

Regarding the stuff about argument formatting, I'll leave it for now but I think aim to consolidate everything at some point - right now there are also problems like ignored arguments and inconsistent argument names, so there should definitely be a break to fix it (and then after that keep it stable).

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