Skip to content

bpo-41621: More accurate signature for defaultdict#21945

Merged
terryjreedy merged 6 commits into
python:mainfrom
sweeneyde:dddoc
Jun 22, 2021
Merged

bpo-41621: More accurate signature for defaultdict#21945
terryjreedy merged 6 commits into
python:mainfrom
sweeneyde:dddoc

Conversation

@sweeneyde
Copy link
Copy Markdown
Member

@sweeneyde sweeneyde commented Aug 24, 2020 •
edited by bedevere-bot
Loading

default_factory cannot be passed as a keyword argument.

https://bugs.python.org/issue41621

@sweeneyde sweeneyde requested a review from rhettinger as a code owner August 24, 2020 13:21
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Aug 24, 2020
iritkatriel
iritkatriel reviewed Aug 24, 2020
View reviewed changes
Comment thread Modules/_collectionsmodule.c
@rhettinger
Copy link
Copy Markdown
Contributor

rhettinger commented Aug 24, 2020

I believe there was a decision to not use the slash notation in the main docs because users found it to be unintelligible. For example, len() isn't documented as len(obj, /) and sorted() isn't documented as sorted(iterable, /, *, key=None, reverse=False).

These have been used in docstrings but only as a artifact of using the argument clinic.

@tirkarthi
Copy link
Copy Markdown
Member

tirkarthi commented Aug 28, 2020

I believe there was a decision to not use the slash notation in the main docs because users found it to be unintelligible

len and other similar changes were discussed at https://bugs.python.org/issue37134

@sweeneyde
Copy link
Copy Markdown
Member Author

sweeneyde commented Aug 28, 2020

Currently len and sorted have the positional-only notation when using help(), but not in the HTML docs. What if we were to make the same true for defaultdict: add the =None and the slash to the __doc__, but leave the HTML documentation as is, except maybe adding something like

The first (positional) argument provides...
         ^^^^^^^^^^^^^

Perhaps that is an appropriate compromise?

terryjreedy
terryjreedy requested changes Aug 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

bpo-37134 added '/' to the docs for struct.unpack_from/compress/decompress, sum, and bytes/bytearray.translate. These fit under the Steering Council's case 2 (Brett Cannon, message 344753): 'a mixture of positional-only and positional-or-keyword args (i.e. "..., /, ...")'. So does this function: default_factory is positional only, where as additional args passed on to the dict can be either.

>>> dd(None, {1:1})
defaultdict(None, {1: 1})
>>> dd(None, one=1)
defaultdict(None, {'one': 1})

Approve except for the indicated minor change.

Comment thread Doc/library/collections.rst Outdated
@bedevere-bot

This comment has been minimized.

Sign in to view
@sweeneyde @terryjreedy
Update Doc/library/collections.rst
4e03362 
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@domdfcoding
Copy link
Copy Markdown
Contributor

domdfcoding commented Nov 9, 2020

@sweeneyde I see you've made the minor change Terry requested. Is this PR ready to be reviewed again?

@sweeneyde
Copy link
Copy Markdown
Member Author

sweeneyde commented Nov 11, 2020

Yes, I forgot about this.

I have made the requested changes; please review again.

I think this change is worthwhile because unlike len(obj=17), which fails immediately, defaultdict(default_factory=list) silently gives unexpected behavior.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 11, 2020

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

abitrolly
abitrolly reviewed May 31, 2021
View reviewed changes
Comment thread Doc/library/collections.rst
@terryjreedy
Copy link
Copy Markdown
Member

terryjreedy commented Jun 22, 2021 •
edited
Loading

Tests/macOS: test_ssl failed.
Azure Pipelines: "Azure DevOps services are currently unavailable."

@terryjreedy terryjreedy merged commit d1ae570 into python:main Jun 22, 2021
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 22, 2021

Thanks @sweeneyde for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 22, 2021
@sweeneyde @miss-islington
bpo-41621: Document defaultdict's default_factory parameter (pythonGH…
d09b18d 
…-21945)

It defaults to None and is positional only.
(cherry picked from commit d1ae570)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 22, 2021

GH-26850 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 22, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 22, 2021
@sweeneyde @miss-islington
bpo-41621: Document defaultdict's default_factory parameter (pythonGH…
29e1e4a 
…-21945)

It defaults to None and is positional only.
(cherry picked from commit d1ae570)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 22, 2021

GH-26851 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit that referenced this pull request Jun 22, 2021
@miss-islington @sweeneyde
bpo-41621: Document defaultdict's default_factory parameter (GH-21945)
a65df3f 
It defaults to None and is positional only.
(cherry picked from commit d1ae570)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
terryjreedy pushed a commit that referenced this pull request Jun 23, 2021
@miss-islington @sweeneyde
bpo-41621: Document defaultdict's default_factory parameter (GH-21945)
88a3342 
It defaults to None and is positional only.
(cherry picked from commit d1ae570)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
@sweeneyde sweeneyde deleted the dddoc branch January 25, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedy terryjreedy terryjreedy approved these changes

@iritkatriel iritkatriel iritkatriel approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

+1 more reviewer

@abitrolly abitrolly abitrolly left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs Documentation in the Doc dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@sweeneyde @rhettinger @tirkarthi @bedevere-bot @domdfcoding @terryjreedy @miss-islington @iritkatriel @abitrolly @the-knights-who-say-ni