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.

Sign up for GitHub

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

Jump to bottom

gh-116938: Clarify documentation of dict and dict.update regarding the positional argument they accept #125213

Merged
merged 3 commits into from
Oct 11, 2024
Merged

gh-116938: Clarify documentation of dict and dict.update regarding the positional argument they accept #125213

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d72b2e
gh-116938: Clarify documentation of `dict` and `dict.update` regardin…
Viicos Oct 9, 2024
06f9ddb
Apply feedback and update docstring of `MutableMapping.update`
Viicos Oct 11, 2024
b486ecc
Update stdtypes.rst
AlexWaygood Oct 11, 2024
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: 13 additions & 12 deletions Doc/library/stdtypes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4505,14 +4505,14 @@ can be used interchangeably to index the same dictionary entry.
``dict([('foo', 100), ('bar', 200)])``, ``dict(foo=100, bar=200)``

If no positional argument is given, an empty dictionary is created.
If a positional argument is given and it is a mapping object, a dictionary
is created with the same key-value pairs as the mapping object. Otherwise,
the positional argument must be an :term:`iterable` object. Each item in
the iterable must itself be an iterable with exactly two objects. The
first object of each item becomes a key in the new dictionary, and the
second object the corresponding value. If a key occurs more than once, the
last value for that key becomes the corresponding value in the new
dictionary.
If a positional argument is given and it defines a ``keys()`` method, a
dictionary is created by calling :meth:`~object.__getitem__` on the argument with
each returned key from the method. Otherwise, the positional argument must be an
:term:`iterable` object. Each item in the iterable must itself be an iterable
with exactly two objects. The first element of each item becomes a key in the
new dictionary, and the second element the corresponding value. If a key occurs
more than once, the last value for that key becomes the corresponding value in
the new dictionary.

If keyword arguments are given, the keyword arguments and their values are
added to the dictionary created from the positional argument. If a key
Expand Down Expand Up @@ -4669,10 +4669,11 @@ can be used interchangeably to index the same dictionary entry.
Update the dictionary with the key/value pairs from *other*, overwriting
existing keys. Return ``None``.

:meth:`update` accepts either another dictionary object or an iterable of
key/value pairs (as tuples or other iterables of length two). If keyword
arguments are specified, the dictionary is then updated with those
key/value pairs: ``d.update(red=1, blue=2)``.
:meth:`update` accepts either another object with a ``keys()`` method (in
which case :meth:`~object.__getitem__` is called with every key returned from
the method). or an iterable of key/value pairs (as tuples or other iterables
of length two). If keyword arguments are specified, the dictionary is then
updated with those key/value pairs: ``d.update(red=1, blue=2)``.
Comment on lines +4672 to +4676
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a premature period on line 4674 after closing brace.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Want to file a PR to fix it?

Copy link
Contributor

Choose a reason for hiding this comment

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

#125421


.. method:: values()

Expand Down
2 changes: 1 addition & 1 deletion Lib/_collections_abc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,7 @@ def clear(self):

def update(self, other=(), /, **kwds):
''' D.update([E, ]**F) -> None. Update D from mapping/iterable E and F.
If E present and has a .keys() method, does: for k in E: D[k] = E[k]
If E present and has a .keys() method, does: for k in E.keys(): D[k] = E[k]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per #116938 (comment)

If E present and lacks .keys() method, does: for (k, v) in E: D[k] = v
In either case, this is followed by: for k, v in F.items(): D[k] = v
Comment on lines +965 to 967
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would like it to be more specific:

Suggested change
If E present and has a .keys() method, does: for k in E.keys(): D[k] = E[k]
If E present and lacks .keys() method, does: for (k, v) in E: D[k] = v
In either case, this is followed by: for k, v in F.items(): D[k] = v
If E present and has keys attribute, does: for k in E.keys(): D[k] = E[k]
If E present and lacks keys attribute, does: for (k, v) in E: D[k] = v
In either case, this is followed by: for k, v in F.items(): D[k] = v

In the web docs the requirements are strict:

  • dict: If a positional argument is given and it defines a keys() method, <...>. Otherwise, the positional argument must be an iterable object.
  • update: Accepts either another object with a keys() method <...> or <...>.

Usage of words must and accept indicate that any object with non-callable keys is not a valid argument. But phrase If E present and lacks .keys() method does not eliminate cases with non-callable keys.
For example:

class Object:
    keys = []
    def __iter__(self, /):
        return iter(self.keys)

This class clearly lacks .keys() method because keys is a list, but passing it to dict.update results in TypeError: 'list' object is not callable.

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 fine IMO. It's much less important for docstrings to be completely precise than it is for the ReST documentation. The most important thing for the docstrings is for them to be concise and easily readable in a terminal or editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Any particular reason why is there no a before .keys() method in the second sentence? It is present in 3.12, but looks like it was removed prior this PR.
image

Copy link
Member

Choose a reason for hiding this comment

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

You're calling help(dict.update) there, which has a different docstring to collections.abc.Mapping.update (the one being edited here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my bad. In that case dict.update should also be updated as it works the same way (i.e. it calls keys attribute).

Copy link
Member

Choose a reason for hiding this comment

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

Right, but the docstring for dict.update already appears to be correct, unless I'm misunderstanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR the first sentence was updated from for k in E: D[k] = E[k] to for k in E.keys(): D[k] = E[k] for MutableMapping. dict.update has the same behavior and its docstring has the same flaw, so it would be correct to fix it too.

cpython/Objects/dictobject.c

Lines 4521 to 4525 in 2982bdb

PyDoc_STRVAR(update__doc__,
"D.update([E, ]**F) -> None. Update D from dict/iterable E and F.\n\
If E is present and has a .keys() method, then does: for k in E: D[k] = E[k]\n\
If E is present and lacks a .keys() method, then does: for k, v in E: D[k] = v\n\
In either case, this is followed by: for k in F: D[k] = F[k]");

Consider:

class Object:
    def keys(self):
        return [1, 2]
    def __iter__(self):
        return [3, 4]
    def __getitem__(self, item):
        match item:
            case 1: return 'one'
            case 2: return 'two'
            case 3: return 'three'
            case 4: return 'four'
        
        return item


print(dict(Object()))
d = {}
d.update(Object())
print(d)

Both times the result is {1: 'one', 2: 'two'}, i.e., both times keys() is called, the object is not iterated.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I see what you mean now. Yes, I suppose that could also be clarified.

'''
Expand Down
Loading