Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
hugovk commented Dec 11, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Doc/library/random.rst Outdated
| 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 |
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.
I am not sure that it is right to replace references to Random methods by references to global functions.
@rhettinger, what do you think?
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.
@serhiy-storchaka I also think the proposed edit is incorrect.
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.
Okay, how's this? 4bbac89
Or should they be separately documented?
Uh oh!
There was an error while loading. Please reload this page.
rhettinger commented Dec 13, 2023
@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? |
Uh oh!
There was an error while loading. Please reload this page.
hugovk commented Dec 13, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. ![]() ![]() And the HTML markup is identical:
Edit: An alternative is to add documentation for these class methods, so the links render correctly. Is this preferable? |
serhiy-storchaka commented Dec 13, 2023
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 of |
AlexWaygood commented Dec 13, 2023
I think Hugo is correct here. When it comes to methods such as
|
serhiy-storchaka commented Dec 13, 2023
Remember, that there is always the 0th variant. |
hugovk commented Dec 13, 2023
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: Is there another way to silence the warnings? (Other than |
hugovk commented Dec 20, 2023
@rhettinger Which of Alex's suggestions do you prefer? |
hugovk commented Dec 20, 2023
Ah sorry, I just noticed you had assigned this to @AlexWaygood. @AlexWaygood, which do you prefer? |
AlexWaygood commented Dec 21, 2023
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 customise |
hugovk commented Dec 26, 2023
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: |
AlexWaygood commented Dec 28, 2023
Thanks, this looks much better now! What do you think about doing something like this, so that all the information on subclassing --- 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.++ |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hugovk commented Dec 28, 2023
Looks good, updated. Also added the missing signatures, checked against the source. |
AlexWaygood left a comment
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, thanks!
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
hugovk commented Dec 28, 2023
Thanks all for the reviews! |
Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
GH-113551 is a backport of this pull request to the 3.12 branch. |
GH-113552 is a backport of this pull request to the 3.11 branch. |
…2981) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
…2981) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
…2981) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>






Fix 7 Sphinx warnings:
And update a couple of URL redirects.
📚 Documentation preview 📚: https://cpython-previews--112981.org.readthedocs.build/en/112981/library/random.html