Skip to content

gh-103673: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer socketservers #103674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 24, 2023
Merged

gh-103673: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer socketservers #103674

merged 11 commits into from
Apr 24, 2023
+19 −10

Conversation

jb2170
Copy link
Contributor

@jb2170 jb2170 commented Apr 22, 2023 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot mentioned this pull request Apr 22, 2023
Closed
@sobolevn sobolevn changed the title gh-103673 gh-103673: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer socketservers Apr 22, 2023
sobolevn
sobolevn reviewed Apr 22, 2023
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the feature suggestion and the implementation.

Please, to increase the chances of this feature been accepted, add tests and docs: when to use these new types of servers, how, etc.

@gpshead gpshead self-assigned this Apr 23, 2023
@jb2170
Copy link
Contributor Author

jb2170 commented Apr 24, 2023

add tests

I have now updated Lib/test/test_socketserver.py to include the appropriate tests. Turns out it was defining its own ForkingUnix{Stream,Datagram}Server within the test file exactly as we are adding to socketserver, who would've guessed hahaha.

and docs

The current docs already describe in detail the ThreadingMixIn and ForkingMixIn, when to use them etc. The only thing that really needed adding was the mentions of the new servers at the 'These classes are pre-defined using the mix-in classes' section. The existing documentation only mentions the {Forking,Threading}{TCP,UDP}Server classes once in this section, and the new docs in this PR are consistent with this; the documentation seems enough.

Thanks for the intermediary commits. The PR is now up-to-date again with main and is conflict free.

@gpshead gpshead enabled auto-merge (squash) April 24, 2023 22:06
@gpshead gpshead merged commit d94b3a6 into python:main Apr 24, 2023
@jb2170 jb2170 deleted the gh-103673 branch April 24, 2023 22:48
carljm added a commit to carljm/cpython that referenced this pull request Apr 24, 2023
@carljm
Merge branch 'main' into superopt_spec
3236f45 
* main:
  pythongh-103801: Tools/wasm linting and formatting (python#103796)
  pythongh-103673: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer socketservers (python#103674)
  pythongh-95795: Move types.next_version_tag to PyInterpreterState (pythongh-102343)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn left review comments

@gpshead gpshead gpshead approved these changes

Assignees

@gpshead gpshead

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jb2170 @gpshead @sobolevn @bedevere-bot @arhadthedev