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

bpo-37564: ArgumentParser support bool type according to truth values #14709

Open
wants to merge 3 commits into
base: master
from

Conversation

@zachben
Copy link

commented Jul 11, 2019

@the-knights-who-say-ni

This comment has been minimized.

Copy link

commented Jul 11, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@mangrisano
Copy link
Contributor

left a comment

LGTM. Thank you for providing test :)

Show resolved Hide resolved Lib/argparse.py Outdated
Replace the import of strtobool from distutils.util with local implem…
…entation of this method that returns bool type instead of int 0/1.
@hpaulj

This comment has been minimized.

Copy link

commented Jul 15, 2019

bool already exists as a Python function. It should not be used as a registry key. This proposal should not shadow a builtin function without clearly documenting it. It should be clear to users (even novices) that they are using a documented function, 'strtobool' that converts some strings to boolean. We should not use a slight-of-hand just to make input of 'True/False' more convenient.

@zachben

This comment has been minimized.

Copy link
Author

commented Jul 15, 2019

bool already exists as a Python function. It should not be used as a registry key. This proposal should not shadow a builtin function without clearly documenting it. It should be clear to users (even novices) that they are using a documented function, 'strtobool' that converts some strings to boolean. We should not use a slight-of-hand just to make input of 'True/False' more convenient.

Hi Paul,
I appreciate the points of concern that you have raised, which are indeed solid and with undoubtful seniority, and I would like to express my (and many others) point of view.
As a matter of fact, I wanted to reply in the bpo-37564 issue but couldn't figure out how to do that, so I will respond here.
It is clear and true that the bool function is intended to work as described in the documentation you referred to, and as you have mentioned in the StackOverflow thread.
But this has nothing to do with the notion of defining CLI that should be interacted by a user, and as such, it should provide a simple, logical, and intuitive UI. The Python developer is using ArgumentParser to implement such CLI, and he/she deserves an intuitive interface, to reflect the expected user experience. We could not expect millions of developers to duplicate code that wraps this utility, just to gain this simple behavior.

Telling this developer that 'type' parameter is anything else then the object-type of the retrieved argument value from the user is not intuitive, and misleading.
We should be bold enough to admit historical mistakes, and fix them for the sake of millions of developers that spending precious time on StackOverflow, where they could have been avoid it.
The documentation for parameter 'type' in the init method of class Action, proves that from the first place bool type has been neglected to be not useful, and it's a shame.

It is agreed that the 'type' parameter is supposed to be callable. This is to allow a user to provide custom conversion functions for his/her special needs. But when it comes to primitive types such as int, str, float, and bool - the 'type' parameter should simply define the type of which the value will be received from the (humane) user.

No humane user will think to provide an empty string to express Falae, or to provide the string 'False' in order to express True.
And no developer will think to configure type=bool and at the same time to expect the user to behave as described above.

Regarding the qualms that you have raised about the proposed solution:

  • The 'store_true' and 'store_false' actions cannot replace a simplified true/false parameter, that in some cases it's value needed to be generated by a wrapping script.
  • I removed the 'bool' wrapper by implementing strtobool locally with return type adjustment.
  • I removed the extra import also by implementing strtobool locally.
  • This type should definitely be registered as object bool and not as string 'bool' from the reasons explained above.

I also agree that this solution might break a consistent behavior, although I don't see any reason why someone will have in his/her legacy code type=bool, while it is obviously not serving their needs.
Your observation is super critical here, so please let me know what do you think now, after reading this, fairly long :), explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.