-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
gh-100989: Fix docstrings of collections.deque
#100990
gh-100989: Fix docstrings of collections.deque
#100990
Conversation
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. |
6a66e8a
to
072515c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
072515c
to
d5a4557
Compare
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!
c8824dc
to
47ceeed
Compare
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!
cae4944
to
eb1219d
Compare
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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.
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 |
@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 The smallest possible changes that would resolve the warning would be to also replace I still think that if the documentation of
|
@timoludwig: thanks for the explanation! I'll review it with this in mind. |
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. |
Thanks @timoludwig for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11. |
…pythonGH-100990) (cherry picked from commit c740736) Co-authored-by: Timo Ludwig <ti.ludwig@web.de> Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
GH-102910 is a backport of this pull request to the 3.11 branch. |
…pythonGH-100990) (cherry picked from commit c740736) Co-authored-by: Timo Ludwig <ti.ludwig@web.de> Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
GH-102911 is a backport of this pull request to the 3.10 branch. |
…cstrings (python#100990)" This reverts commit c740736.
…cstrings (python#100990)" This reverts commit c740736.
…python#100990) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
…python#100990) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
See gh-100989 for the issue description.
Thanks in advance for your feedback!