Repository navigation
Add 'bool' format character to PyArg_ParseTuple* #58910
Description
| BPO | 14705 |
|---|---|
| Nosy | @loewis, @birkenfeld, @rhettinger, @mdickinson, @larryhastings, @ericvsmith, @benjaminp, @serhiy-storchaka |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/larryhastings'
closed_at = <Date 2012-05-06.06:03:22.058>
created_at = <Date 2012-05-02.09:29:19.125>
labels = ['interpreter-core', 'type-feature']
title = "Add 'bool' format character to PyArg_ParseTuple*"
updated_at = <Date 2012-05-07.09:45:44.331>
user = 'https://github.com/larryhastings'bugs.python.org fields:
activity = <Date 2012-05-07.09:45:44.331>
actor = 'python-dev'
assignee = 'larry'
closed = True
closed_date = <Date 2012-05-06.06:03:22.058>
closer = 'larry'
components = ['Interpreter Core']
creation = <Date 2012-05-02.09:29:19.125>
creator = 'larry'
dependencies = []
files = ['25441', '25453', '25465', '25468']
hgrepos = []
issue_num = 14705
keywords = ['patch']
message_count = 34.0
messages = ['159781', '159783', '159785', '159839', '159844', '159916', '159918', '159923', '159926', '159927', '159928', '159929', '159930', '159931', '159932', '159933', '159934', '160000', '160001', '160003', '160005', '160006', '160007', '160008', '160013', '160016', '160019', '160044', '160045', '160046', '160047', '160049', '160056', '160127']
nosy_count = 9.0
nosy_names = ['loewis', 'georg.brandl', 'rhettinger', 'mark.dickinson', 'larry', 'eric.smith', 'benjamin.peterson', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14705'
versions = ['Python 3.3']
Activity
larryhastings commentedon May 2, 2012
Currently functions that parse their arguments with the PyArg_ParseTuple family which want to take a boolean-ish parameter face two choices:
The former is foolish; though it works with ints and bools, it doesn't work with any other type (float, str, list, etc) which strictly speaking are valid for boolean fields. And this is common enough that the latter should not be necessary.
I propose to add support for a new format character to the PyArg_ParseTuple family: 'b', which specifies 'boolean'. Its
implementation would be much the same as that of 'd' except:
If we cared, I could also add 'B', which would only accept
either Py_True or Py_False. But I see no reason why we'd ever want
to strictly enforce the type... do you? (I can see MvL's argument now:
"We've lived this long without it. YAGNI.")
If there's interest I'll knock out a patch. I expect it to be less than ten lines not counting documentation and test. (Less than twenty if
folks actually want 'B'.)
serhiy-storchaka commentedon May 2, 2012
Yes, I too have encountered this in the process of working on bpo-14626. For this purpose it is appropriate to use a special converter (with 'O&'). I suppose it must be a strict сonverter; there is no point in specifying followlinks=[1, 2, 3]. If it is somewhere need a nonstrict ones -- use 'O' and then check the how to in this particular case.
Would be good to have a special letter for this format, but note that 'b' and 'B' are already used. It is better first to use the сonverter, and expand the format only when the enough number of uses.
larryhastings commentedon May 2, 2012
The converter works--but, then, a similar converter would also work
for double, and float, and long long and many others. If long long
is special enough to merit explicit support in PyArg_ParseTuple,
then bool definitely is, as I strongly suspect bool is used much more
frequently.
I don't see a need for that either, but--where would you draw the line? What is the principle that says "ints are okay, floats are... okay, lists are not"?
Python has a long-established concept of the "truthiness" of an expression. I *strongly* suggest we stick with that unless we have a *very* good reason why not.
I'm a moron! I must have looked right at it and it didn't register.
't' (truth) and 'f' (false) are also taken. As is 'c' (conditional).
The only thing I can come up with that's remotely related and
isn't already taken is 'p' for predicate.
I suspect there are already loads of places in the standard library
that should be using this rather than abusing 'i'.
larryhastings commentedon May 3, 2012
My first patch. Adds 'p' and 'P', along with documentation and unit tests.
serhiy-storchaka commentedon May 3, 2012
Patch looks good to me.
I however prefer to use 'P'.
ericvsmith commentedon May 4, 2012
The patch looks good to me.
Are there any places in the current code base that would use "P"? "p" seems the more useful case.
Are you planning on changing existing code to use P or p, or just use it going forward?
mdickinson commentedon May 4, 2012
I also think that 'P' looks too strict to be really useful. I can't think of too many cases where I'd really want to insist on a boolean argument (and reject values of 0 or 1).
Maybe implement just 'p' for now and consider 'P' later?
larryhastings commentedon May 4, 2012
I looked through the Python sources and couldn't find any instances of a function or method with an argument that only allowed you to pass in either True or False.
Serily already said he would use 'P' over 'p', although I too am unconvinced that's a good idea. Serily: why would you unhesitatingly prefer 'P' to 'p'?
Certainly I see loads of uses for 'p'. For example, when converting code from Python to C that already relied on Python's standard definition of truthiness.
I did find some spots that took an object and converted to bool with PyObject_IsTrue, like _json.Encoder(allow_nan) and pickle._Pickler(fix_imports). These too would be well-served by 'p'.
I also found some funny in-between cases. For example, stat_float_times and the three-argument form of os.symlink both claim to take a boolean but actually take 'i' (integer). This is relying on bool.__int__(). We certainly couldn't use 'P' here. We could consider switching these to 'p', though in all likelyhood we'll just leave 'em alone.
loewis commentedon May 4, 2012
I think there should be a test case also where PyObject_IsTrue gives an exception (which I think can happen if __bool__ raises an exception).
larryhastings commentedon May 4, 2012
I'd be happy to add such a test, but I don't know of any types like that. Can anyone suggest one?
loewis commentedon May 4, 2012
larryhastings commentedon May 4, 2012
Added test forcing a failure for 'p' (and 'P'). This made me have to handle errors a little differently, so it was definitely good to test it. Thanks for the suggestion, Martin!
Also changed wording in documentation ever-so-slightly.
9 remaining items
mdickinson commentedon May 5, 2012
Not an issue: bool can't be subclassed. :-)
serhiy-storchaka commentedon May 5, 2012
I just guided by the principle "Explicit is better than implicit".
Statements 'if' and 'while' I consider explicit and expect cast to bool.
Implicit cast to bool in the transfer of parameter causes discomfort (no
one (no one Pythonist) expects the implicit cast to str). Unfortunately,
for historical reasons, there is a lot of code, where the parameters are
casted to bool or int is used instead of bool. Therefore, we cannot
simply enter the restrictions.
Well, I was certainly wrong. Don't let me mislead you.
Class bool cannot be subclassed further.
larryhastings commentedon May 5, 2012
Serhiy, I'm having a little trouble with your English. (But I'm sure your English is far better than my... uh, anything besides English.) So let me ask a question with a clear yes/no answer:
Do you still think 'P' is better than 'p'?
serhiy-storchaka commentedon May 5, 2012
No.
I hope I haven't made a lot of mistakes in the previous sentence.
larryhastings commentedon May 5, 2012
It depends, do you consider three "a lot"? ;-)
Attached is a new patch removing 'P'. (The regrtest is still running but I don't expect any failures.) I'm guessing I won't get any further feedback. So unless I hear otherwise I'll check it in tomorrow.
mdickinson commentedon May 5, 2012
Latest patch looks good to me.
python-dev commentedon May 5, 2012
New changeset bc6d28e726d8 by Larry Hastings in branch 'default':
Issue bpo-14705: Add 'p' format character to PyArg_ParseTuple* for bool support.
http://hg.python.org/cpython/rev/bc6d28e726d8
larryhastings commentedon May 5, 2012
Eh, it was ready, why wait? Thanks everybody for your feedback!
rhettinger commentedon May 6, 2012
I would have expected a bool parse code to insist on a boolean, so that:
would behave the same as:
larryhastings commentedon May 6, 2012
I originally had a second form ('P') that insisted on a boolean as you suggest. But nobody could come up with a use case. So I removed it in the final patch. Please see this issue for the discussion.
If you have a use case for it I'd be happy to revive it and check it in.
python-dev commentedon May 6, 2012
New changeset 709850f1ec67 by Larry Hastings in branch 'default':
Update Misc/NEWS for issues bpo-14127 and bpo-14705. (And, technically, bpo-10148.)
http://hg.python.org/cpython/rev/709850f1ec67
python-dev commentedon May 6, 2012
New changeset 05274ab06182 by Larry Hastings in branch 'default':
Update Misc/NEWS for issues bpo-14127 and bpo-14705. (And, technically, bpo-10148.)
http://hg.python.org/cpython/rev/05274ab06182
python-dev commentedon May 7, 2012
New changeset e4617650f006 by Larry Hastings in branch 'default':
Issue bpo-14705: Added support for the new 'p' format unit to skipitem().
http://hg.python.org/cpython/rev/e4617650f006