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

Jump to bottom

bpo-41621: More accurate signature for defaultdict #21945

Merged
merged 6 commits into from Jun 22, 2021
Merged

bpo-41621: More accurate signature for defaultdict #21945

merged 6 commits into from Jun 22, 2021

Conversation

sweeneyde
Copy link
Member

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

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 Aug 24, 2020
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Aug 24, 2020
@rhettinger
Copy link
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
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
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 changes
Copy link
Member

@terryjreedy terryjreedy left a comment

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.

Doc/library/collections.rst Outdated Show resolved Hide resolved
@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
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
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

bedevere-bot commented Nov 11, 2020

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from terryjreedy Nov 11, 2020
@terryjreedy
Copy link
Member

terryjreedy commented Jun 22, 2021 •
edited

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

@terryjreedy terryjreedy merged commit d1ae570 into python:main Jun 22, 2021
11 of 13 checks passed
@miss-islington
Copy link
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

bedevere-bot commented Jun 22, 2021

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

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

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 Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abitrolly abitrolly

@terryjreedy terryjreedy

@iritkatriel iritkatriel

@rhettinger rhettinger

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.

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