-
-
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
gh-88427: Deprecate socket.SocketType
#126272
base: main
Are you sure you want to change the base?
Conversation
socket.SocketType
socket.SocketType
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.
SocketType
isn't deprecated right now, so we can't remove it yet.
Please update the PR to raise a DeprecationWarning
when accessing a SocketType
(you can do this via a PEP-562 __getattr__
), and then add the documentation notes.
Maybe we can wait for the promoter to answer. I won't open this PR before I decide whether to abandon or remove it. |
I looked at the code again and agree with my opinion from 2021: it should be deprecated and eventually removed. It could be useful to do a code search (e.g., GitHub code search, Hugo's top 5k pypi packages tool) to see how the name is being used. A possible approach to deprecation could be to remove the current line adding it to |
Okay, I'll re-revise this PR so that it throws a warning when calling this type. Call me back in this PR when we consider removing it in the future :) |
Misc/NEWS.d/next/Library/2024-11-01-20-09-53.gh-issue-88427.chYNT6.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2024-11-01-20-09-53.gh-issue-88427.chYNT6.rst
Outdated
Show resolved
Hide resolved
It seems that the module |
It works for any kind of attributes. Please, read PEP-562 for examples on how to use it. |
I read the document and modified it. But why does it trigger this
Edit |
I think you need to raise an |
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.
@rruuaanng: Did you try your change before updating the PR? It doesn't work, no warning is emitted:
$ ./python
>>> import _socket
>>> _socket.SocketType
<class '_socket.socket'>
I suggest to mark the PR as a draft since it changes many times in a short time.
@vstinner
|
Right. Please test your change before publishing it. |
Please allow me to check again to see if there are any unresolved comments to decide whether we should remove the |
test_socket and test_pickle are failing on the CI Ubuntu. |
Oh.... I don't understand, I was looking for what was throwing the error so I ran the failing tests locally and it seemed like they all worked fine on my computer. test_socket.py
test_pickle.py
I tried removing my changes and ran
Edit
|
PyErr_Warn(PyExc_DeprecationWarning, "_socket.SocketType is deprecated and " | ||
"will be removed in Python 3.16. " | ||
"Use socket.socket instead"); | ||
return state != NULL ? (PyObject *)state->sock_type : NULL; |
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.
Should sock_type
get INCREF
'd? I'm assuming its a static type and therefore immortal, but we should add a comment clarifying that.
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 tried to search for situations where sock_type
is used in socketmodule.c
. In the scenario where Py_DECREF
is used, only socket_clear
uses Py_DECREF
. And sock_type
does not use Py_INCREF
. And in other parts, it seems that there is no special comment.
cc @JelleZijlstra