bpo-32320: Add default value support to collections.namedtuple() #4859
Conversation
| @@ -805,6 +805,13 @@ they add the ability to access fields by name instead of position index. | |||
| converted to ``['abc', '_1', 'ghi', '_3']``, eliminating the keyword | |||
| ``def`` and the duplicate fieldname ``abc``. | |||
|
|
|||
| *defaults* can be ``None`` or an :term:`iterable` of default values. | |||
There was a problem hiding this comment.
Wouldn't the interface be simpler if make the default value of defaults an empty tuple?
There was a problem hiding this comment.
It might be simpler but that isn't the norm of Python standard library APIs and I don't really like the way it looks.
There was a problem hiding this comment.
This is the norm. Run git grep '=[(][)]' and you will get numerous examples.
| .. doctest:: | ||
|
|
||
| >>> Account = namedtuple('Account', ['type', 'balance'], defaults=[0]) | ||
| >>> Account._fields_defaults |
There was a problem hiding this comment.
I think it is better to demonstrate the purposed effect of having default values:
>>> Account('premium')
Account(type='premium', balance=0)
_fields_defaults is for advanced users, which are able to read the documentation and help carefully. The ability to omit arguments in the constructor is the purpose of using the defaults argument.
There was a problem hiding this comment.
That makes sense. I've expanded the example
| defaults = tuple(defaults) | ||
| if len(defaults) > len(field_names): | ||
| raise TypeError('Got more default values than field names') | ||
| field_defaults = dict(reversed(list(zip(reversed(field_names), |
There was a problem hiding this comment.
Wouldn't the following code be more clear?
if defaults:
field_defaults = dict(zip(field_names[-len(defaults)], defaults))There was a problem hiding this comment.
I don't like that the zero-length case has to be guarded against. It may be a matter of taste, but I prefer the current code over negative indexing.
There was a problem hiding this comment.
But your code already contains special guards for None. You could just change the one of guards.
| @@ -332,7 +332,8 @@ def namedtuple(typename, field_names, *, rename=False, module=None): | |||
| if isinstance(field_names, str): | |||
| field_names = field_names.replace(',', ' ').split() | |||
| field_names = list(map(str, field_names)) | |||
| typename = str(typename) | |||
| typename = _sys.intern(str(typename)) | |||
There was a problem hiding this comment.
This is not needed.
If typename is not valid name, ValueError will be raised below. But it already have seat in the interned string repository.
There was a problem hiding this comment.
The typename does not become automatically interned with type(typename), (tuple,), namespace):
>>> import sys
>>> tn = 'Po'
>>> tn1 = tn + 'int'
>>> tn2 = tn + 'i' + 'nt'
>>> t1 = type(tn1, (), {})
>>> t2 = type(tn2, (), {})
>>> t1.__name__ is t2.__name__
False
So, I do think it it wise to intern the typename.
Lib/test/test_collections.py
Outdated
| with self.assertRaises(TypeError): # non-iterable defaults | ||
| Point = namedtuple('Point', 'x y', defaults=10) | ||
|
|
||
| Point = namedtuple('Point', 'x y', defaults=None) # default is None |
There was a problem hiding this comment.
Add a test for empty defaults.
| with self.assertRaises(TypeError): # catch too few args | ||
| Point(10) | ||
|
|
||
| Point = namedtuple('Point', 'x y', defaults=[10, 20]) # allow non-tuple iterable |
There was a problem hiding this comment.
Is it worth to add a test for non-reiterable value, e.g. iter([10, 20])?
| with self.assertRaises(TypeError): # too many defaults | ||
| Point = namedtuple('Point', 'x y', defaults=(10, 20, 30)) | ||
| with self.assertRaises(TypeError): # non-iterable defaults | ||
| Point = namedtuple('Point', 'x y', defaults=10) |
There was a problem hiding this comment.
Add a test for a false non-iterable value like defaults=False.
There was a problem hiding this comment.
Okay. Added another test.
| exec(s, namespace) | ||
| __new__ = namespace['__new__'] | ||
| __new__.__doc__ = f'Create new instance of {typename}({arg_list})' | ||
| if defaults is not None: | ||
| __new__.__defaults__ = defaults |
There was a problem hiding this comment.
could make the default defaults=() and then avoid this branch here and always assign __new__.__defaults__
There was a problem hiding this comment.
That would work. I prefer the current approach though.
https://bugs.python.org/issue32320