Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-101100: Fix Sphinx warnings in library/random.rst#112981

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
hugovk merged 12 commits intopython:mainfromhugovk:docs-fix-sphinx-warnings-random
Dec 28, 2023

Conversation

hugovk
Copy link
Member

@hugovkhugovk commentedDec 11, 2023
edited
Loading

Fix 7 Sphinx warnings:

SPHINXERRORHANDLING=-n PATH=./venv/bin:$PATH sphinx-build -b html -d build/doctrees -j auto -n. build/html ./library/random.rst2>&1| grep WARNING| tee>(wc -l)Doc/library/random.rst:36: WARNING: py:meth reference target not found: Random.randomDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.seedDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.getstateDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.setstateDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.getrandbitsDoc/library/random.rst:416: WARNING: py:class reference target not found: NoneTypeDoc/library/random.rst:443: WARNING: py:meth reference target not found: Random.random       7

And update a couple of URL redirects.


📚 Documentation preview 📚:https://cpython-previews--112981.org.readthedocs.build/en/112981/library/random.html

Comment on lines 37 to 39
basic generator of your own devising: in that case, override the :meth:`~random.random`,
:meth:`~random.seed`, :meth:`~random.getstate`, and :meth:`~random.setstate` methods.
Optionally, a new generator can supply a :meth:`~random.getrandbits` method --- this

Choose a reason for hiding this comment

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

I am not sure that it is right to replace references to Random methods by references to global functions.

@rhettinger, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@serhiy-storchaka I also think the proposed edit is incorrect.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Okay, how's this?4bbac89

Or should they be separately documented?

@rhettinger
Copy link
Contributor

@AlexWaygood can you take a look at this and make a suggestion. Is there a way to address the Sphinx warnings without damaging the existing correct useful links that are currently being rendered?

@hugovk
Copy link
MemberAuthor

hugovk commentedDec 13, 2023
edited
Loading

@AlexWaygood can you take a look at this and make a suggestion. Is there a way to address the Sphinx warnings without damaging the existing correct useful links that are currently being rendered?

This PR doesn't break any existing links.

The existing links are not actually links: the warnings indicate that they fail to render, because there are no docs at the target location.

The PR changes links these broken links to unlinked method names, which render the same, and do not issue any reference warnings.

Before:

image

After:

image

And the HTML markup is identical:

BeforeAfter
imageimage

Edit: An alternative is to add documentation for these class methods, so the links render correctly. Is this preferable?

AlexWaygood reacted with thumbs up emoji

@serhiy-storchaka
Copy link
Member

Is there a way to address the Sphinx warnings without damaging the existing correct useful links that are currently being rendered?

Currently these references do not render as links, because targets are not defined. This PR replaces the method references with the global function references, which will be rendered as links. I.e. instead ofseed() there will beseed(). On one hand, working links are good, on other hand, they refer to global functions rather than Random methods. Methods of Random subclass are purposed to be overridden, but "overriding" global functions is something different. This can cause confusion. But it is the only place whereseed() is described, there is no separate documentation for methods. Links allow you to see signature and semantic of the methods. So there are pluses and minuses in these changes. I'm not going to decide which prevails and leave it up to you,@rhettinger.

AlexWaygood reacted with thumbs up emoji

@AlexWaygood
Copy link
Member

I think Hugo is correct here. When it comes to methods such asrandom.Random.seed, which currently do not have a canonical anchor for us to link to, we have three options, as far as I can see:

  1. Instead of telling Sphinx to add a link torandom.Random.seed (which it can't do at the moment), tell Sphinx to add a link torandom.seed(). This is what Hugo did in his first commit.
  2. Tell Sphinx not to bother trying to add a link torandom.Random.seed. This is what the PR currently proposes.
  3. Add a canonical anchor for us to link to whenrandom.Random.seed is referenced, that is separate torandom.seed. This is possibly the most principled course of action, but will involve some degree of duplication, sincerandom.Random.seed() andrandom.seed() do basically the same thing. (But not necessarily too much duplication -- we could simply document the method with a one-line entry: "Seerandom.seed".)
willingc reacted with thumbs up emoji

@serhiy-storchaka
Copy link
Member

  1. (the current status) Keep it as silenced Sphinx warnings, as a reminder to solve it later, when we will have opportunity.

Remember, that there is always the 0th variant.

@hugovk
Copy link
MemberAuthor

  1. (the current status) Keep it as silenced Sphinx warnings, as a reminder to solve it later, when we will have opportunity.

I'm willing to solve it now, if only we can decide how to solve it :)

The first thing I tried was like this:

-:meth:`~Random.random`+:meth:`~!Random.random`

They render the same as the original, but we still get warnings:

Doc/library/random.rst:36: WARNING: py:meth reference target not found: Random.randomDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.seedDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.getstateDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.setstateDoc/library/random.rst:36: WARNING: py:meth reference target not found: Random.getrandbitsDoc/library/random.rst:443: WARNING: py:meth reference target not found: Random.random

Is there another way to silence the warnings?

(Other than!Random.random, which renders asRandom.random() instead ofrandom().

@rhettingerrhettinger removed their request for reviewDecember 13, 2023 20:52
@hugovk
Copy link
MemberAuthor

@rhettinger Which of Alex's suggestions do you prefer?

@hugovk
Copy link
MemberAuthor

Ah sorry, I just noticed you had assigned this to@AlexWaygood.

@AlexWaygood, which do you prefer?

@AlexWaygood
Copy link
Member

Ah sorry, I just noticed you had assigned this to@AlexWaygood.

@AlexWaygood, which do you prefer?

I quite like the idea of option (3). I think adding a short stub entry for each of these methods might be useful, -- maybe just 1-2 sentences for each. Something like "Override this method in subclasses to customiseblah behaviour ofRandom instances".

hugovk reacted with thumbs up emoji

@hugovk
Copy link
MemberAuthor

Thanks, updated to add stub entries:

https://cpython-previews--112981.org.readthedocs.build/en/112981/library/random.html#random.Random

Also, in some examples, move the comments in so they fit without needing to scroll horizontally:

Details

Before

image

https://docs.python.org/3/library/random.html#examples

After

image

https://cpython-previews--112981.org.readthedocs.build/en/112981/library/random.html#examples

AlexWaygood and mattwang44 reacted with heart emoji

@AlexWaygoodAlexWaygood self-requested a reviewDecember 26, 2023 11:22
@AlexWaygood
Copy link
Member

Thanks, this looks much better now! What do you think about doing something like this, so that all the information on subclassingrandom.Random is in one place?

--- a/Doc/library/random.rst+++ b/Doc/library/random.rst@@ -34,10 +34,8 @@ instance of the :class:`random.Random` class.  You can instantiate your own instances of :class:`Random` to get generators that don't share state. Class :class:`Random` can also be subclassed if you want to use a different-basic generator of your own devising: in that case, override the :meth:`~Random.random`,-:meth:`~Random.seed`, :meth:`~Random.getstate`, and :meth:`~Random.setstate` methods.-Optionally, a new generator can supply a :meth:`~Random.getrandbits` method --- this-allows :meth:`randrange` to produce selections over an arbitrarily large range.+basic generator of your own devising: see the documentation on that class for+more details. The :mod:`random` module also provides the :class:`SystemRandom` class which uses the system function :func:`os.urandom` to generate random numbers@@ -412,6 +410,9 @@ Alternative Generator       ``None``, :class:`int`, :class:`float`, :class:`str`,       :class:`bytes`, or :class:`bytearray`.+   Subclasses of :class:`!Random` should override the following methods if they+   wish to make use of a different basic generator:+    .. method:: Random.seed()       Override this method in subclasses to customise the :meth:`random.seed`@@ -427,16 +428,21 @@ Alternative Generator       Override this method in subclasses to customise the :meth:`random.setstate`       behaviour of :class:`Random` instances.-   .. method:: Random.getrandbits()--      Override this method in subclasses to customise the :meth:`random.getrandbits`-      behaviour of :class:`Random` instances.-    .. method:: Random.random()       Override this method in subclasses to customise the :meth:`random.random`       behaviour of :class:`Random` instances.+   Optionally, a new generator can also supply the following method:++   .. method:: Random.getrandbits()++      Override this method in subclasses to customise the :meth:`random.getrandbits`+      behaviour of :class:`Random` instances. This in turn allows+      :meth:`!Random.randrange` to produce selections over an arbitrarily large+      range.++
hugovk reacted with thumbs up emoji

@hugovk
Copy link
MemberAuthor

Thanks, this looks much better now! What do you think about doing something like this, so that all the information on subclassingrandom.Random is in one place?

Looks good, updated. Also added the missing signatures, checked against the source.

AlexWaygood reacted with heart emoji

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

hugovk reacted with hooray emoji
hugovkand others added2 commitsDecember 28, 2023 10:23
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@hugovk
Copy link
MemberAuthor

Thanks all for the reviews!

AlexWaygood reacted with rocket emoji

@hugovkhugovk merged commit8e5d70f intopython:mainDec 28, 2023
@hugovkhugovk deleted the docs-fix-sphinx-warnings-random branchDecember 28, 2023 19:29
@miss-islington-app
Copy link

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

@bedevere-app
Copy link

GH-113551 is a backport of this pull request to the3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestDec 28, 2023
…112981)(cherry picked from commit8e5d70f)Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelDec 28, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestDec 28, 2023
…112981)(cherry picked from commit8e5d70f)Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bedevere-app
Copy link

GH-113552 is a backport of this pull request to the3.11 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.11only security fixes labelDec 28, 2023
hugovk added a commit that referenced this pull requestDec 28, 2023
… (#113551)Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
hugovk added a commit that referenced this pull requestDec 28, 2023
… (#113552)Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
kulikjak pushed a commit to kulikjak/cpython that referenced this pull requestJan 22, 2024
aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@rhettingerrhettingerrhettinger left review comments

@willingcwillingcwillingc left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

Assignees

@AlexWaygoodAlexWaygood

Labels
docsDocumentation in the Doc dirskip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@hugovk@rhettinger@serhiy-storchaka@AlexWaygood@willingc

[8]ページ先頭

©2009-2025 Movatter.jp