Skip to content

GH-101100: Fix reference warnings for socket methods #110114

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

Merged
merged 10 commits into from
Nov 27, 2023

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Sep 29, 2023

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I think that in all cases where it refers to socket() as a function, it should keep :func: role.

@@ -23,7 +23,7 @@ all modern Unix systems, Windows, MacOS, and probably additional platforms.

The Python interface is a straightforward transliteration of the Unix system
call and library interface for sockets to Python's object-oriented style: the
:func:`.socket` function returns a :dfn:`socket object` whose methods implement
:class:`.socket` function returns a :dfn:`socket object` whose methods implement
Copy link
Member

Choose a reason for hiding this comment

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

It says "the socket() function". I think that it is better to leave references to class as a function in the context where the fact that it is a callable is more important than the fact that it is also a type. There are a lot of precedences with int(), str(), range(), map(), etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you suggest adding a .. function:: socket directive (potentially just above .. class:: socket)?

Copy link
Member

Choose a reason for hiding this comment

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

It is not needed, the role type should not affect links. For example :func:`int` and :class:`int` refer to the same .. class:: declaration. The difference is that :func: adds ().

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this locally (replacing all :class:`.socket` with :func:`socket` ) -- the problem is that the cross-reference goes to the module object definition (library/socket.html#module-socket) rather than the socket callable.

So I think our best option is to add a .. function:: socket directive.

A

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking further, socket() was previously documented as a function, but #23960 changed it to be documented as a class. Perhaps we could change it back? map is documented as a function, for example.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I think that the problem is that if you use a reference starting with ., Sphinx switches in more strict mode that checks the role type, so :func:`.socket` does not find the class declaration. But :func:`socket.socket` may work.

@hugovk
Copy link
Member

hugovk commented Oct 31, 2023

@AA-Turner Please could you resolve the conflicts? Then I think we're ready to merge.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I restored the use of the .. class:: directive for socket. It fixes rendering for "the socket constructor". The :func: references work when specify the full qualified name.

Also fixed references for few more methods.

@hugovk hugovk merged commit ffe1b2d into python:main Nov 27, 2023
@miss-islington-app
Copy link

Thanks @AA-Turner 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 Nov 27, 2023
…nGH-110114)

(cherry picked from commit ffe1b2d)

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 27, 2023
…nGH-110114)

(cherry picked from commit ffe1b2d)

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 27, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 27, 2023
@bedevere-app
Copy link

bedevere-app bot commented Nov 27, 2023

GH-112456 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 Nov 27, 2023
hugovk pushed a commit that referenced this pull request Nov 27, 2023
hugovk pushed a commit that referenced this pull request Nov 27, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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.

4 participants