Skip to content

gh-101100: [docs] Fix Sphinx warnings in dis module#102597

Closed
sobolevn wants to merge 3 commits into
python:mainfrom
sobolevn:issue-101100-dis
Closed

gh-101100: [docs] Fix Sphinx warnings in dis module#102597
sobolevn wants to merge 3 commits into
python:mainfrom
sobolevn:issue-101100-dis

Conversation

@sobolevn

@sobolevn sobolevn commented Mar 11, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Original errors:

./cpython/Doc/library/dis.rst:46: WARNING: py:func reference target not found: myfunc
./cpython/Doc/library/dis.rst:51: WARNING: py:func reference target not found: myfunc
./cpython/Doc/library/dis.rst:795: WARNING: py:func reference target not found: builtins.__build_class__
./cpython/Doc/library/dis.rst:852: WARNING: py:attr reference target not found: co_names
./cpython/Doc/library/dis.rst:859: WARNING: py:attr reference target not found: co_names
./cpython/Doc/library/dis.rst:898: WARNING: py:attr reference target not found: co_names
./cpython/Doc/library/dis.rst:907: WARNING: py:attr reference target not found: co_names
./cpython/Doc/library/dis.rst:1426: WARNING: c:func reference target not found: PyObject_Format

A couple of notes for reviewers:

  1. :func:myfunc was rendered incorrectly (invalid signature):

    Снимок экрана 2023-03-11 в 12 02 24

    Because :func: is reserved for functions defined with .. function:: directive.

  2. I've silenced !builtins.__build_class__ reference, because as far as I understand, there's no desire in documenting this feature. So, now this reference renders the same way, but just does not error out

  3. :attr:!codeobject.* are also just ignoring warnings, because right now there's no way to address these attributes documented here: https://github.com/python/cpython/blame/5ffdaf748d98da6065158534720f1996a45a0072/Doc/reference/datamodel.rst#L956 So, we just add an ignore and slightly more context

  4. PyObject_Format will be found after gh-102595: Document PyObject_Format c-api function #102596 is merged

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir skip news labels Mar 11, 2023
@bedevere-bot bedevere-bot mentioned this pull request Mar 11, 2023
Open
CAM-Gerlach
CAM-Gerlach reviewed Mar 11, 2023
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems otherwise good overall, but instead of disabling resolution and silencing the warnings, could co_names be properly documented instead, just like codeobject.co_positions currently is, especially since it's referenced nearly a dozen times in this document?

@sobolevn

sobolevn commented Mar 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

I don't know enough about the history of these fields. They might be not documented on purpose. We need a field expert here to decide.

@sobolevn sobolevn requested a review from iritkatriel March 25, 2023 07:45
@sobolevn

sobolevn commented Mar 25, 2023

Copy link
Copy Markdown
Member Author

@iritkatriel can you please take a look? :)

@JelleZijlstra JelleZijlstra self-requested a review May 7, 2023 02:07
@JelleZijlstra JelleZijlstra added the needs backport to 3.11 only security fixes label May 7, 2023
AA-Turner
AA-Turner requested changes May 8, 2023
View reviewed changes

@AA-Turner AA-Turner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A comment on the pre-fixing of codeobject. -- I think we should revert this for now.

It may also be better to add an entry to nitpick_ignore in conf.py with an explanatory note rather than supressing these warnings, if we intend to document the code object attributes at a later date.

Comment thread Doc/library/dis.rst Outdated
@hugovk

hugovk commented May 8, 2023

Copy link
Copy Markdown
Member

if we intend to document the code object attributes at a later date.

Or can we do it now?

@JelleZijlstra JelleZijlstra removed their request for review June 7, 2023 00:49
@sobolevn @AA-Turner
Update Doc/library/dis.rst
3887849 
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@sobolevn

sobolevn commented Aug 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Closed by #108476 by @erlend-aasland

@sobolevn sobolevn closed this Aug 25, 2023
@erlend-aasland

erlend-aasland commented Aug 27, 2023

Copy link
Copy Markdown
Contributor

Sorry, @sobolevn, I didn't know you had a PR open :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@arhadthedev arhadthedev arhadthedev left review comments

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@AA-Turner AA-Turner AA-Turner requested changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@iritkatriel iritkatriel Awaiting requested review from iritkatriel

Assignees

No one assigned

Labels

awaiting merge docs Documentation in the Doc dir needs backport to 3.11 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@sobolevn @hugovk @erlend-aasland @JelleZijlstra @arhadthedev @AA-Turner @CAM-Gerlach @Fidget-Spinner @bedevere-bot