FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-41621: More accurate signature for defaultdict by sweeneyde · Pull Request #21945 · python/cpython · GitHub

/ cpython Public

bpo-41621: More accurate signature for defaultdict - #21945

Merged
terryjreedy merged 6 commits into
python:mainfrom
sweeneyde:dddoc
Jun 22, 2021
ConversationCommits (6)ChecksFiles changed
Merged

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

Conversation

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

Copy link
Copy Markdown
Member

default_factory cannot be passed as a keyword argument.

https://bugs.python.org/issue41621

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

rhettinger commented Aug 24, 2020

Copy link
Copy Markdown
Contributor

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 commented Aug 28, 2020

Copy link
Copy Markdown
Member

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 commented Aug 28, 2020

Copy link
Copy Markdown
Member Author

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 requested changes Aug 29, 2020
View reviewed changes

terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

This comment has been minimized.

Sign in to view
Update Doc/library/collections.rst
4e03362 
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>

domdfcoding commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

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

sweeneyde commented Nov 11, 2020

Copy link
Copy Markdown
Member Author

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 commented Nov 11, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

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

abitrolly reviewed May 31, 2021
View reviewed changes
Comment thread Doc/library/collections.rst

terryjreedy commented Jun 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

terryjreedy merged commit d1ae570 into python:main Jun 22, 2021

miss-islington commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

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
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 commented Jun 22, 2021

Copy link
Copy Markdown

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

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
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 commented Jun 22, 2021

Copy link
Copy Markdown

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
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
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 mentioned this pull request Jul 18, 2021
Closed
sweeneyde deleted the dddoc branch January 25, 2022 23:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

terryjreedy terryjreedy approved these changes
iritkatriel iritkatriel approved these changes
rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
+1 more reviewer
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

Back | FazBrowse Home | New Git URL