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

gh-106033: [docs] Improve C API GetItem & HasAttr notes. #106047

Merged
merged 15 commits into from
Jun 24, 2023

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Jun 23, 2023

This should format the notes and call out the preferred APIs to use in a more visible manner.


📚 Documentation preview 📚: https://cpython-previews--106047.org.readthedocs.build/

This should format the notes and call out the preferred APIs to use
in a more visible manner.
@gpshead gpshead added docs Documentation in the Doc dir skip news needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Jun 23, 2023
@gpshead gpshead requested review from CAM-Gerlach and removed request for CAM-Gerlach June 23, 2023 19:39
@gpshead gpshead marked this pull request as ready for review June 23, 2023 19:39
@gpshead gpshead requested a review from CAM-Gerlach June 23, 2023 19:39
@gpshead gpshead self-assigned this Jun 23, 2023
@gpshead
Copy link
Member Author

gpshead commented Jun 23, 2023

In the docs preview these notes render well and are much more obvious than the previous plain text paragraph:

https://cpython-previews--106047.org.readthedocs.build/en/106047/c-api/dict.html#c.PyDict_GetItem
https://cpython-previews--106047.org.readthedocs.build/en/106047/c-api/object.html#c.PyObject_HasAttr

I changed the "suppressed" wording to "silently ignored" but I'm not sure that is any better. swallowed, eaten, and discarded also come to mind. thoughts?

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a 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 by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit

Two sets of suggestions:

  • Given this is cautioning/warning against problematic behavior, I suggest using the appropriate admonition type accordingly.
  • Fix a number of broken references (to generic dunder methods) introduced in this PR.

Other than that, LGTM, thanks! It is definitely an improvement, as you say.

Doc/c-api/dict.rst Show resolved Hide resolved
Doc/c-api/dict.rst Outdated Show resolved Hide resolved
Doc/c-api/dict.rst Outdated Show resolved Hide resolved
Doc/c-api/dict.rst Show resolved Hide resolved
Doc/c-api/dict.rst Outdated Show resolved Hide resolved
Doc/c-api/object.rst Outdated Show resolved Hide resolved
Doc/c-api/object.rst Show resolved Hide resolved
Doc/c-api/object.rst Show resolved Hide resolved
Doc/c-api/object.rst Outdated Show resolved Hide resolved
Doc/c-api/object.rst Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

I changed the "suppressed" wording to "silently ignored" but I'm not sure that is any better. swallowed, eaten, and discarded also come to mind. thoughts?

Not a C API expert of course, but to me "silently ignored" sounded more appropriate in terms of affect/implications than "suppressed", since the latter implies a deliberate, purposeful act while the former better conveys this is a potentially undesired side effect. "Swallowed" and "eaten" are potentially more negative, but also more informal for technical documentation and potentially decreases the gravity of the note, and "discarded" sounds somewhere between "suppressed" and "silently ignored" to me, or about the same, so I'm fine with "silently ignored" personally.

gpshead and others added 12 commits June 23, 2023 17:08
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
ironically stands out less than note, but the bold caution: is still sufficient and better than what we had.  warning would stand out too much.

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
ironically stands out less than note, but the bold caution: is still sufficient and better than what we had.  warning would stand out too much.

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Would be LGTM, but it seems the Python docs theme is entirely missing appropriate styles for caution admonitions—they are not only lacking an appropriate color, but are entirely missing the whole callout box around them, too.

Therefore, until that is fixed, you'll have to either go back to using note, or step it up to warning (as is used elsewhere).

@gpshead gpshead merged commit 19d6511 into python:main Jun 24, 2023
@miss-islington
Copy link
Contributor

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@gpshead gpshead deleted the gh-106033-docs branch June 24, 2023 23:29
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2023
…nGH-106047)

Use a note:: tag so that these dict and object API deficiencies show up clearly.

A caution:: tag was considered, but our current python docs rendering doesn't do much with that (no box or color change).  warning:: seemed too extreme.  note looks good.
(cherry picked from commit 19d6511)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2023
…nGH-106047)

Use a note:: tag so that these dict and object API deficiencies show up clearly.

A caution:: tag was considered, but our current python docs rendering doesn't do much with that (no box or color change).  warning:: seemed too extreme.  note looks good.
(cherry picked from commit 19d6511)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@bedevere-bot
Copy link

GH-106070 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 24, 2023
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 24, 2023
gpshead added a commit that referenced this pull request Jun 24, 2023
…06047) (#106071)

gh-106033: [docs] Improve C API GetItem & HasAttr notes. (GH-106047)

Use a note:: tag so that these dict and object API deficiencies show up clearly.

A caution:: tag was considered, but our current python docs rendering doesn't do much with that (no box or color change).  warning:: seemed too extreme.  note looks good.
(cherry picked from commit 19d6511)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit that referenced this pull request Jun 24, 2023
…06047) (#106070)

gh-106033: [docs] Improve C API GetItem & HasAttr notes. (GH-106047)

Use a note:: tag so that these dict and object API deficiencies show up clearly.

A caution:: tag was considered, but our current python docs rendering doesn't do much with that (no box or color change).  warning:: seemed too extreme.  note looks good.
(cherry picked from commit 19d6511)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants