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-109510: Clearly explain "Which Docstrings Are Examined" #109696

Merged
merged 15 commits into from Oct 19, 2023

Conversation

Unique-Usman
Copy link
Contributor

@Unique-Usman Unique-Usman commented Sep 21, 2023

The "Which Docstrings Are Examined" part of
Doctest documentation is ambiguos, This change explained it cleary with an example.


📚 Documentation preview 📚: https://cpython-previews--109696.org.readthedocs.build/en/109696/library/doctest.html#which-docstrings-are-examined

The "Which Docstrings Are Examined" part of
Doctest documentation is ambiguos, This change explained it cleary with an example.
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Sep 21, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app bot added awaiting review docs Documentation in the Doc dir skip news labels Sep 21, 2023
@AlexWaygood AlexWaygood changed the title #109510: Clearly explained "Which Docstrings Are Examined" gh-109510: Clearly explained "Which Docstrings Are Examined" Sep 21, 2023
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>
@Unique-Usman

This comment was marked as off-topic.

@Unique-Usman

This comment was marked as off-topic.

@zware
Copy link
Member

zware commented Oct 4, 2023

@python/proofreaders, could we take a look here?

@bskinn
Copy link
Contributor

bskinn commented Oct 5, 2023

Can you clarify your proofreading ask here, @zware?

If I read the thread right, the new content is not done yet, and the advice being sought is w.r.t. what to include, not how to write/format it.

@zware
Copy link
Member

zware commented Oct 5, 2023

Sorry, could have clarified :). I meant to request proofreaders as a reviewer, but GitHub didn't give me the option to do so.

I recommended to @Unique-Usman to leave the further improvements to another PR rather than holding this one up. As such, I've marked the messages about further changes as 'off-topic'.

@Unique-Usman
Copy link
Contributor Author

thank @zware as you recommended, I will be creating a new PR for the further improvement.

Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
The value of ``example.__test__["numbers"]`` value will be treated as
docstring and all the tests inside it will be run. It is also
important to note that the value can also be mapped to a Function,
class object or module. If the value is a class, Function or module, doctest
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class object or module. If the value is a class, Function or module, doctest
class object or module. If the value is a class, Function or module, :mod:`doctest`

Do we link?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. We use the appropriate role to get the appropriate formatting, but add ! to not resolve the link, since it is to the current module docs that the reader is already on, which isn't particularly helpful.

Unique-Usman and others added 3 commits October 12, 2023 01:47
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Changed doctest :mod: `doctest`
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Unique-Usman and others added 3 commits October 12, 2023 02:49
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@CAM-Gerlach CAM-Gerlach changed the title gh-109510: Clearly explained "Which Docstrings Are Examined" gh-109510: Clearly explain "Which Docstrings Are Examined" Oct 11, 2023
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.

Hey, thanks for this. Looks good overall; I had some relatively modest feedback, all as suggestions you can apply. You can easily do so all in one go by going to Files, clicking Add to batch on each suggestion, and once done clicking Commit. Thanks!

Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Doc/library/doctest.rst Outdated Show resolved Hide resolved
Accepted suggestions to meet python documentation convention

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@Unique-Usman
Copy link
Contributor Author

@CAM-Gerlach Thanks for this it really helped.

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.

LGTM overall, but I do have one last semi-substantive thing I'm confused on, which my previous changes ended up highlighting. Thanks!

Doc/library/doctest.rst Outdated Show resolved Hide resolved
Unique-Usman and others added 2 commits October 18, 2023 10:43
Changed :samp:`{name of M}.__test__.K` to ``M.__test__.K`` as the M is already a placeholder for name of the module.

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Doc/library/doctest.rst Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Oct 18, 2023

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.

Changed the `True` to "is true" to specify that ``M.__test__`` has a truthy value, rather than literally being True.
@Unique-Usman
Copy link
Contributor Author

I have made the requested changes; please review again

Doc/library/doctest.rst Outdated Show resolved Hide resolved
Changes "is true" to "is truthy" to imply that the value of ``M.__test__" has true value.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

@hugovk hugovk added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Oct 19, 2023
@hugovk hugovk merged commit bcc941b into python:main Oct 19, 2023
25 of 26 checks passed
@miss-islington-app
Copy link

Thanks @Unique-Usman for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 19, 2023
…honGH-109696)

(cherry picked from commit bcc941b)

Co-authored-by: Unique-Usman <86585626+Unique-Usman@users.noreply.github.com>
Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 19, 2023
…honGH-109696)

(cherry picked from commit bcc941b)

Co-authored-by: Unique-Usman <86585626+Unique-Usman@users.noreply.github.com>
Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@bedevere-app
Copy link

bedevere-app bot commented Oct 19, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 19, 2023
@bedevere-app
Copy link

bedevere-app bot commented Oct 19, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Oct 19, 2023
hugovk added a commit that referenced this pull request Oct 19, 2023
…-109696) (#111077)

Co-authored-by: Unique-Usman <86585626+Unique-Usman@users.noreply.github.com>
Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
hugovk added a commit that referenced this pull request Oct 19, 2023
…-109696) (#111078)

Co-authored-by: Unique-Usman <86585626+Unique-Usman@users.noreply.github.com>
Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@Unique-Usman Unique-Usman deleted the unique branch October 20, 2023 19:53
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…hon#109696)

Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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.

None yet

8 participants