-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-30052: DOC: Link bytes
to stdtypes not functions
#1271
Conversation
@csabella, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @ncoghlan and @berkerpeksag to be potential reviewers. |
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.
This looks like a good fix for the original issue to me (as it brings bytes/bytearray cross-references into line with the other builtin container types).
I think we should merge this to dev and backport as far as 3.6, but will leave it open for a day or so to give others a chance to comment.
Doc/library/stdtypes.rst
Outdated
|
||
.. classmethod:: bytes.fromhex(string) | ||
.. classmethod:: bytes.fromhex(string) |
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.
bytes.
prefix is unnecessary now.
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.
Thank you. I wasn't sure about these. Fixed it.
Doc/library/stdtypes.rst
Outdated
|
||
.. method:: bytes.hex() | ||
.. method:: bytes.hex() |
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.
bytes.
prefix is unnecessary now.
Doc/library/stdtypes.rst
Outdated
|
||
>>> bytearray(b'\xf0\xf1\xf2').hex() | ||
'f0f1f2' | ||
.. method:: bytearray.hex() |
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.
bytearray.
prefix is unnecessary now.
(like ``b'abc'``) and the built-in function :func:`bytes` can be used to | ||
construct bytes objects. Also, bytes objects can be decoded to strings | ||
via the :meth:`~bytes.decode` method. | ||
(like ``b'abc'``) and the built-in :func:`bytes()` constructor |
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.
Why did you add () here? Perhaps using :ref:`bytes <func-bytes>` would be better?
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 added the () to be similar language to set() just below this on the same page. I had originally changed it to func-butes, but the other sequences on the page didn't refer the the functions.rst, but to the stdtypes.rst, so I erred on the side of keeping them consistent. Should I change it?
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.
Thanks for the explanation. I'll let @ncoghlan decide which one is more appropriate here. If we decide to go with a reference to functions.rst, the "()" part should be removed (it's redundant, Sphinx will automatically add ()s in the HTML output)
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.
This is in the data model section, so referencing the class documentation rather than the builtin function entry makes sense to me.
Merged, thanks @csabella! @berkerpeksag Having merged it, it belatedly occurs to me to ask: do you think we should add a NEWS entry for this? It shouldn't break any incoming links, so it isn't necessary from that perspective, and looking at past Documentation NEWS entries, they seem to mostly be related to documented deprecations, major content additions, and changes to the docs build process. |
@ncoghlan +1, a Misc/NEWS entry is not needed in this case. |
…pythonGH-1271) Builtin container types have two potential link targets in the docs: - their entry in the list of builtin callables - their type documentation This change brings `bytes` and `bytearray` into line with other container types by having cross-references default to linking to their type documentation, rather than their builtin callable entry.. (cherry picked from commit c6db481)
) (GH-1915) Builtin container types have two potential link targets in the docs: - their entry in the list of builtin callables - their type documentation This change brings `bytes` and `bytearray` into line with other container types by having cross-references default to linking to their type documentation, rather than their builtin callable entry.. (cherry picked from commit c6db481)
Change for :class:
bytes
and :class:bytesarray
to link to the stdtypes page instead of the functions page.Note: to check all the links, I needed to use 'make clean' then 'make html'. I don't know if that would be done automatically when merging.