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-32320: Add default value support to collections.namedtuple() #4859

Merged
merged 4 commits into from
Jan 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion Doc/library/collections.rst
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ Named tuples assign meaning to each position in a tuple and allow for more reada
self-documenting code. They can be used wherever regular tuples are used, and
they add the ability to access fields by name instead of position index.

.. function:: namedtuple(typename, field_names, *, rename=False, module=None)
.. function:: namedtuple(typename, field_names, *, rename=False, defaults=None, module=None)

Returns a new tuple subclass named *typename*. The new subclass is used to
create tuple-like objects that have fields accessible by attribute lookup as
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the interface be simpler if make the default value of defaults an empty tuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the norm. Run git grep '=[(][)]' and you will get numerous examples.

Since fields with a default value must come after any fields without a
default, the *defaults* are applied to the rightmost parameters. For
example, if the fieldnames are ``['x', 'y', 'z']`` and the defaults are
``(1, 2)``, then ``x`` will be a required argument, ``y`` will default to
``1``, and ``z`` will default to ``2``.

If *module* is defined, the ``__module__`` attribute of the named tuple is
set to that value.

Expand All @@ -824,6 +831,10 @@ they add the ability to access fields by name instead of position index.
.. versionchanged:: 3.7
Remove the *verbose* parameter and the :attr:`_source` attribute.

.. versionchanged:: 3.7
Added the *defaults* parameter and the :attr:`_field_defaults`
attribute.

.. doctest::
:options: +NORMALIZE_WHITESPACE

Expand Down Expand Up @@ -911,6 +922,18 @@ field names, the method and attribute names start with an underscore.
>>> Pixel(11, 22, 128, 255, 0)
Pixel(x=11, y=22, red=128, green=255, blue=0)

.. attribute:: somenamedtuple._fields_defaults

Dictionary mapping field names to default values.

.. doctest::

>>> Account = namedtuple('Account', ['type', 'balance'], defaults=[0])
>>> Account._fields_defaults
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I've expanded the example

{'balance': 0}
>>> Account('premium')
Account(type='premium', balance=0)

To retrieve a field whose name is stored in a string, use the :func:`getattr`
function:

Expand Down
20 changes: 17 additions & 3 deletions Lib/collections/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ def __eq__(self, other):

_nt_itemgetters = {}

def namedtuple(typename, field_names, *, rename=False, module=None):
def namedtuple(typename, field_names, *, rename=False, defaults=None, module=None):
"""Returns a new subclass of tuple with named fields.

>>> Point = namedtuple('Point', ['x', 'y'])
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.


if rename:
seen = set()
for index, name in enumerate(field_names):
Expand All @@ -342,6 +343,7 @@ def namedtuple(typename, field_names, *, rename=False, module=None):
or name in seen):
field_names[index] = f'_{index}'
seen.add(name)

for name in [typename] + field_names:
if type(name) is not str:
raise TypeError('Type names and field names must be strings')
Expand All @@ -351,6 +353,7 @@ def namedtuple(typename, field_names, *, rename=False, module=None):
if _iskeyword(name):
raise ValueError('Type names and field names cannot be a '
f'keyword: {name!r}')

seen = set()
for name in field_names:
if name.startswith('_') and not rename:
Expand All @@ -360,6 +363,14 @@ def namedtuple(typename, field_names, *, rename=False, module=None):
raise ValueError(f'Encountered duplicate field name: {name!r}')
seen.add(name)

field_defaults = {}
if defaults is not None:
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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the following code be more clear?

if defaults:
    field_defaults = dict(zip(field_names[-len(defaults)], defaults))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But your code already contains special guards for None. You could just change the one of guards.

reversed(defaults)))))

# Variables used in the methods and docstrings
field_names = tuple(map(_sys.intern, field_names))
num_fields = len(field_names)
Expand All @@ -372,10 +383,12 @@ def namedtuple(typename, field_names, *, rename=False, module=None):

s = f'def __new__(_cls, {arg_list}): return _tuple_new(_cls, ({arg_list}))'
namespace = {'_tuple_new': tuple_new, '__name__': f'namedtuple_{typename}'}
# Note: exec() has the side-effect of interning the typename and field names
# Note: exec() has the side-effect of interning the field names
exec(s, namespace)
__new__ = namespace['__new__']
__new__.__doc__ = f'Create new instance of {typename}({arg_list})'
if defaults is not None:
__new__.__defaults__ = defaults
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could make the default defaults=() and then avoid this branch here and always assign __new__.__defaults__

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work. I prefer the current approach though.


@classmethod
def _make(cls, iterable):
Expand Down Expand Up @@ -420,6 +433,7 @@ def __getnewargs__(self):
'__doc__': f'{typename}({arg_list})',
'__slots__': (),
'_fields': field_names,
'_fields_defaults': field_defaults,
'__new__': __new__,
'_make': _make,
'_replace': _replace,
Expand Down
51 changes: 51 additions & 0 deletions Lib/test/test_collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,57 @@ def test_factory(self):
self.assertRaises(TypeError, Point._make, [11]) # catch too few args
self.assertRaises(TypeError, Point._make, [11, 22, 33]) # catch too many args

def test_defaults(self):
Point = namedtuple('Point', 'x y', defaults=(10, 20)) # 2 defaults
self.assertEqual(Point._fields_defaults, {'x': 10, 'y': 20})
self.assertEqual(Point(1, 2), (1, 2))
self.assertEqual(Point(1), (1, 20))
self.assertEqual(Point(), (10, 20))

Point = namedtuple('Point', 'x y', defaults=(20,)) # 1 default
self.assertEqual(Point._fields_defaults, {'y': 20})
self.assertEqual(Point(1, 2), (1, 2))
self.assertEqual(Point(1), (1, 20))

Point = namedtuple('Point', 'x y', defaults=()) # 0 defaults
self.assertEqual(Point._fields_defaults, {})
self.assertEqual(Point(1, 2), (1, 2))
with self.assertRaises(TypeError):
Point(1)

with self.assertRaises(TypeError): # catch too few args
Point()
with self.assertRaises(TypeError): # catch too many args
Point(1, 2, 3)
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for a false non-iterable value like defaults=False.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Added another test.

with self.assertRaises(TypeError): # another non-iterable default
Point = namedtuple('Point', 'x y', defaults=False)

Point = namedtuple('Point', 'x y', defaults=None) # default is None
self.assertEqual(Point._fields_defaults, {})
self.assertIsNone(Point.__new__.__defaults__, None)
self.assertEqual(Point(10, 20), (10, 20))
with self.assertRaises(TypeError): # catch too few args
Point(10)

Point = namedtuple('Point', 'x y', defaults=[10, 20]) # allow non-tuple iterable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth to add a test for non-reiterable value, e.g. iter([10, 20])?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

self.assertEqual(Point._fields_defaults, {'x': 10, 'y': 20})
self.assertEqual(Point.__new__.__defaults__, (10, 20))
self.assertEqual(Point(1, 2), (1, 2))
self.assertEqual(Point(1), (1, 20))
self.assertEqual(Point(), (10, 20))

Point = namedtuple('Point', 'x y', defaults=iter([10, 20])) # allow plain iterator
self.assertEqual(Point._fields_defaults, {'x': 10, 'y': 20})
self.assertEqual(Point.__new__.__defaults__, (10, 20))
self.assertEqual(Point(1, 2), (1, 2))
self.assertEqual(Point(1), (1, 20))
self.assertEqual(Point(), (10, 20))


@unittest.skipIf(sys.flags.optimize >= 2,
"Docstrings are omitted with -O2 and above")
def test_factory_doc_attr(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
collections.namedtuple() now supports default values.