-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
adjusting socketserver.BaseServer
#13082
base: main
Are you sure you want to change the base?
Conversation
This one might be a bad idea. fileno, get_request, and server_bind are documented to exist on BaseServer, but they don't. Of these, only get_request is actually called from the existing implementation of BaseServer (as part of handle_request and serve_forever). In the ideal case, they'd probably all be abstract methods in CPython.
socketserver.BaseServer
This comment has been minimized.
This comment has been minimized.
No mypy primer hits, but I'm not convinced that means this is the right thing to do. |
I opened a CPython issue about it: python/cpython#127209 |
I could also try making them abstract in the stubs, even if CPython doesn't. |
This comment has been minimized.
This comment has been minimized.
Any specific reason to close this? I haven't been very on top of reviewing typeshed PRs (lots of other things keeping me busy) but I do intend to get back to them. |
Yeah, this one has been sitting in draft because I kind of intend to pursue python/cpython#127209 first and see where that goes. This had merge conflicts again, and since I didn't intend to take it out of draft anytime soon I decided to close it instead of fix the merge conflict. I'll get back to it at some point, but I didn't feel great about this particular solution. |
This comment has been minimized.
This comment has been minimized.
Okay, I'm ready to re-open this now. I have an MR out at CPython which this aligns with: python/cpython#127976 The one thing we might want to do differently here in typeshed is that the I was thinking it wasn't useful to lie about the existence of BaseServer.get_request, but then I realized it probably allows mypy to flag if a child class implements it with the wrong signature. That's enough of a benefit to keep it around. Assuming my other MR is accepted, it'll stop being a lie eventually. |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
This one might be a bad idea. fileno, get_request, and server_bind are documented to exist on BaseServer, but they don't. Of these, only get_request is actually called from the existing implementation of BaseServer (as part of handle_request and serve_forever).
In the ideal case, they'd probably all be abstract methods in CPython.