GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-100989: Fix docstrings of `collections.deque` by timobrembeck · Pull Request #100990 · python/cpython

Conversation

@timobrembeck

@timobrembeck timobrembeck commented Jan 12, 2023

Copy link
Copy Markdown
Contributor

See gh-100989 for the issue description.

Thanks in advance for your feedback!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@timobrembeck
timobrembeck requested a review from rhettinger as a code owner January 12, 2023 15:17
@bedevere-bot bedevere-bot added the awaiting review label Jan 12, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 12, 2023
Accommodate Sphinx option by changing docstring return type of "integer" to "int". #100989
Closed
@bedevere-bot

bedevere-bot commented Jan 12, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Jan 12, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timobrembeck

timobrembeck commented Jan 12, 2023

Copy link
Copy Markdown
Contributor Author

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

In my opinion, it doesn't, but let me know if you think the bot is right.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timobrembeck
timobrembeck force-pushed the gh-100989-fix-collections-deque-docstrings branch from 6a66e8a to 072515c Compare February 3, 2023 12:14
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@arhadthedev arhadthedev added skip news extension-modules C modules in the Modules dir labels Feb 3, 2023
@timobrembeck
timobrembeck force-pushed the gh-100989-fix-collections-deque-docstrings branch from 072515c to d5a4557 Compare February 15, 2023 23:42
CAM-Gerlach
CAM-Gerlach reviewed Feb 25, 2023
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Standard reminder: You can directly apply all the suggestions you want in one go with Files changed -> Add to batch -> Commit

This change seems to have value overall, as it not only fixes the downstream Sphinx warnings, but is clearer and more explicit as well as more concise and consistent as to the parameter and return types of these methods (per Diataxis for reference docs), and provides much clearer and more detailed descriptions that fill in key missing details as to what the parameters actually do.

I did have a couple minor suggestions, see comments. Also, it would be good for someone familiar with both the docs and the C-API (like @erlend-aasland ) should probably also take a look at it. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/_collectionsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_collectionsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach CAM-Gerlach added docs Documentation in the Doc dir needs backport to 3.10 needs backport to 3.11 only security fixes labels Feb 25, 2023
@CAM-Gerlach
CAM-Gerlach requested a review from erlend-aasland February 25, 2023 08:07
@CAM-Gerlach CAM-Gerlach mentioned this pull request Feb 25, 2023
Fix all Sphinx reference warnings in the documentation #101100
Closed
@timobrembeck
timobrembeck force-pushed the gh-100989-fix-collections-deque-docstrings branch 2 times, most recently from c8824dc to 47ceeed Compare February 25, 2023 09:53
CAM-Gerlach
CAM-Gerlach approved these changes Feb 25, 2023
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM from my side from one trivial fix to my own typo. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/_collectionsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 25, 2023
@CAM-Gerlach CAM-Gerlach removed the extension-modules C modules in the Modules dir label Feb 25, 2023
@timobrembeck
timobrembeck force-pushed the gh-100989-fix-collections-deque-docstrings branch from cae4944 to eb1219d Compare February 25, 2023 10:08
@timobrembeck @CAM-Gerlach
pythongh-100989: Fix docstrings of collections.deque
eb1219d
Co-authored-by: C.A.M. Gerlach <[email protected]>
erlend-aasland
erlend-aasland requested changes Mar 22, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Seeing that Raymond (the code owner) specifically requested only the integer => int change to be applied, I think it would better to reduce this PR to that specific change only.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Mar 22, 2023
@bedevere-bot

bedevere-bot commented Mar 22, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timobrembeck

timobrembeck commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

Seeing that Raymond (the code owner) specifically requested only the integer => int change to be applied, I think it would better to reduce this PR to that specific change only.

@erlend-aasland Thanks a lot for your thoughts on this. As explained in #100989 (comment), this suggested change would not solve the Sphinx warning, since Sphinx does not recognize the -- pattern as separation of type and description. I would open a ticket on Sphinx' side if this code style is somehow a defined standard other projects should respect, but I did not find any resources on this. Do you know more about this, or in general a guideline docstrings should follow?

The smallest possible changes that would resolve the warning would be to also replace -- by two new lines, do you think this would be justified as well?

I still think that if the documentation of PyDoc_STRVAR tells us that we should write the docstrings according to PEP-7, we should do one of two things:

  1. Actually write the docstrings according to that guideline
  2. Or remove that passage from the docs and either require a (newer?) guideline or state that docstrings don't have to follow any specific formatting (in which case I would suggest that we also don't use the signature-like format function(params) -> return type (but not really), because it makes other tools like e.g. Sphinx' autodoc_docstring_signature think it's a machine parsable format, which it is not in that case).
CAM-Gerlach and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

@timoludwig: thanks for the explanation! I'll review it with this in mind.

CAM-Gerlach reacted with thumbs up emoji timobrembeck reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach

CAM-Gerlach commented Mar 22, 2023

Copy link
Copy Markdown
Member

And just to note, the eventual changes are AFAIK substantially modified and refined from those that the core dev in question initially was hesitant about on first glance. IMO, the much greater value here is not the specific Sphinx or formatting issue, but rather the substantial increase in clarity, consistency and completeness per the Diatxis guidelines for API reference documentation, particularly for the target audience users who may not be experts in the usage of the code in question.

erlend-aasland and timobrembeck reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland approved these changes Mar 22, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Mar 22, 2023
@erlend-aasland
erlend-aasland merged commit c740736 into python:main Mar 22, 2023
@miss-islington

miss-islington commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

Thanks @timoludwig for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Mar 22, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 22, 2023
@timobrembeck @CAM-Gerlach @miss-islington
pythongh-100989: Improve the accuracy of collections.deque docstrings (…
0101217
…pythonGH-100990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
@bedevere-bot

bedevere-bot commented Mar 22, 2023

Copy link
Copy Markdown

GH-102910 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Mar 22, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 22, 2023
@timobrembeck @CAM-Gerlach @miss-islington
pythongh-100989: Improve the accuracy of collections.deque docstrings (…
eb05231
…pythonGH-100990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
@bedevere-bot

bedevere-bot commented Mar 22, 2023

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Mar 22, 2023
@timobrembeck
timobrembeck deleted the gh-100989-fix-collections-deque-docstrings branch March 22, 2023 11:49
miss-islington added a commit that referenced this pull request Mar 22, 2023
@miss-islington @timobrembeck @CAM-Gerlach
gh-100989: Improve the accuracy of collections.deque docstrings (GH-1…
f7dea0c
…00990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington added a commit that referenced this pull request Mar 22, 2023
@miss-islington @timobrembeck @CAM-Gerlach
gh-100989: Improve the accuracy of collections.deque docstrings (GH-1…
100da7c
…00990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
@erlend-aasland erlend-aasland mentioned this pull request Mar 23, 2023
gh-100989: Revert #100990: Improve the accuracy of collections.deque docstrings #102949
Closed
timobrembeck added a commit to timobrembeck/cpython that referenced this pull request Mar 23, 2023
@timobrembeck
Revert "pythongh-100989: Improve the accuracy of collections.deque do…
2ca7083
…cstrings (python#100990)"

This reverts commit c740736.
rhettinger added a commit to rhettinger/cpython that referenced this pull request Mar 23, 2023
@rhettinger
Revert "pythongh-100989: Improve the accuracy of collections.deque do…
1034f65
…cstrings (python#100990)"

This reverts commit c740736.
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@timobrembeck @CAM-Gerlach @Fidget-Spinner
pythongh-100989: Improve the accuracy of collections.deque docstrings (…
3e09860
…python#100990)

Co-authored-by: C.A.M. Gerlach <[email protected]>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@timobrembeck @CAM-Gerlach @warsaw
pythongh-100989: Improve the accuracy of collections.deque docstrings (…
9d65f91
…python#100990)

Co-authored-by: C.A.M. Gerlach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@timobrembeck @bedevere-bot @erlend-aasland @CAM-Gerlach @miss-islington @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.