Argument Clinic: add "nullable ints"

[larryhastings]

BPO	20341
Nosy	@loewis, @birkenfeld, @rhettinger, @larryhastings, @jkloth, @zware, @serhiy-storchaka, @vajrasky, @MojoVampire
Files	larry.nullable.ints.draft larry.nullable.ints.draft.2 larry.nullable.ints.draft.3 larry.nullable.ints.4.txt

^{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 = None
created_at = <Date 2014-01-22.03:18:01.912>
labels = ['extension-modules', 'type-feature', 'expert-argument-clinic']
title = 'Argument Clinic: add "nullable ints"'
updated_at = <Date 2020-03-18.18:40:55.337>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2020-03-18.18:40:55.337>
actor = 'brett.cannon'
assignee = 'larry'
closed = False
closed_date = None
closer = None
components = ['Extension Modules', 'Argument Clinic']
creation = <Date 2014-01-22.03:18:01.912>
creator = 'larry'
dependencies = []
files = ['33605', '33635', '33639', '36165']
hgrepos = []
issue_num = 20341
keywords = []
message_count = 37.0
messages = ['208740', '208741', '208751', '208764', '208766', '208767', '208770', '208771', '208773', '208775', '208777', '208782', '208795', '208808', '208809', '208810', '208813', '208851', '208852', '208866', '208867', '208885', '208919', '208945', '208950', '208998', '209004', '209010', '209013', '209024', '209027', '224309', '224310', '224463', '224464', '224465', '224493']
nosy_count = 10.0
nosy_names = ['loewis', 'georg.brandl', 'rhettinger', 'larry', 'jkloth', 'Yury.Selivanov', 'zach.ware', 'serhiy.storchaka', 'vajrasky', 'josh.r']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20341'
versions = ['Python 3.4']

[larryhastings]

Attached is a *preliminary* patch for Argument Clinic that adds nullable ints. If you use the converter "int(nullable=True), then:

• The type you get back is not int but "nullable_int_t", a structure
  containing three fields: "error", "is_null", and "i". "error" is 1 if
  there was an error in parsing, else 0. "is_null" is 1 if None was
  passed in, else 0. "i" is the integer value if an integer was
  passed in, else undefined.
• You may now use a default value of None. If you do, "is_null" is 1.
• You may still use a default integer. If you do, "i" will be set
  to that.
• If you use neither then obviously it's a required argument.

I'd appreciate just a preliminary review, saying something like
"this is a good idea, keep going," or
"you should change your approach," or
"this is a bad idea and should not be committed," or
"you look very handsome today Larry".

I don't remember who specifically needed the nullable ints, so I just added a bunch of Derby contestants.

Serhiy: I also added a fix for the bug you mentioned in bpo-20294 after it was closed: the docstring for __new__ and __init__ methods now uses the class name instead of the name of the method. Note that this fix doesn't matter much; once I commit a fix for bpo-20189, that signature will always be hidden.

[larryhastings]

I should have mentioned--a sample using nullable ints is in itertoolsmodule.c. longobject.{ch} changed to add the converters, and obviously clinic.py changed to add support for the converters.

[vajrasky]

>>> itertools.repeat.__doc__
'repeat(object, times=None)\nReturns an iterator which returns the object the specified number of times.\n\nIf times is None, returns the object endlessly.'
>>> itertools.repeat('a', times=None)
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
TypeError: __new__() argument 2 must be (unspecified), not None

Also, maybe it's a good idea to add case for None and negative times in itertools.repeat unit test. You can get inspiration from http://bugs.python.org/issue19145

[larryhastings]

The bug you saw was an easy fix; the converter function needs to return 1 (success) from the "== Py_None" branch.

And yes, more unit tests would be good, fixing the documentation would be good too. I did say the patch was nowhere near ready.

I will assume unless I hear otherwise that I should proceed with the patch and flesh out all the different and float types.

[taleinat]

You look very handsome today Larry :)

Regardless, I do think this is a good idea. I've converted several functions that have -1 as a default value for ints where they ideally should have None. Having None as the default will be much clearer and less error prone for users.

[taleinat]

My only comment from reviewing the patch is that you seem to have used NULLABLE_PY_SSIZE_T_INITIALIZE by accident one time in int_converter (in clinic.py).

Other than that, as you already know, make sure to return 1 in both converters in the "if (arg == Py_None)" branches.

[serhiy-storchaka]

I don't remember who specifically needed the nullable ints, so I just added a bunch of Derby contestants.

There are no much need in nullable ints, if they are needed, different use cases require different semantic. I don't think we can now write general converters which will cover different use cases. May be in 3.5, when behavior changes will be possible.

I know only one use case for nullable Py_ssize_t converter (and mmap_convert_ssize_t is much simpler). itertolls.repeat doesn't need it, because it shouldn't accept None.

Let defer this issue until most of code will be converted. Then we can accumulate statistic about nullable in converters.

[serhiy-storchaka]

And thank you for the fix for the __init__ methods. Could you please commit it? The I'll update patches for bz2 and lzma modules.

Looks as Vajrasky Kok has encountered with yet one bug in generated __init__ wrapper(bpo-20185).

[larryhastings]

Then you disagree with Guido, who says that optional integer arguments without a viable publishable default value should accept None to mean "use the default value":

https://mail.python.org/pipermail/python-dev/2014-January/131673.html
https://mail.python.org/pipermail/python-dev/2014-January/131711.html

There are a fair number of places where people are abusing positional
parameters because they can't specify a good default. I plan to put a stop to that, but first I need to give them a viable alternative.

In the specific case of itertools.repeat, I'm going to talk to the maintainer about what he wants to do. The current behavior is too magic for Python and iiuc Guido regards it as a bug. My guess is a default of None, as the author was deliberately preventing you from passing in -1. But we shall see.

[serhiy-storchaka]

I thought it was too late for 3.4, and the conversion should not change the
behavior (we can change the behavior later in 3.5).

[larryhastings]

I guess we should ask Guido for clarification. But I got the impression that we could *gently* tweak the signatures of functions if they were not representable in Python syntax. He thought that giving the parameter of _sha1.sha1() a default value of b'' was fine for example, and in the case of itertools.repeat he considered the behavior so odd he was guessing it was a bug.

[larryhastings]

Actually, the documented behavior of itertools.repeat() is that the "times" argument takes a default of None.

Equivalent to:

 def repeat(object, times=None):
 ...

http://docs.python.org/3/library/itertools.html#itertools.repeat