GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103673: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer socketservers by jb2170 · Pull Request #103674 · python/cpython

Conversation

@jb2170

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Addresses #103673

  • Issue: socketserver: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer servers #103673

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jb2170 added 3 commits April 22, 2023 02:42
@jb2170
socketserver: Add missing ForkingUnixStreamServer and ForkingUnixData…
8a2f842 
…gramServer servers
@jb2170
Add self to Misc/ACKS
364eb77
@jb2170
Add NEWS entry with blurb
849feb2
@bedevere-bot bedevere-bot mentioned this pull request Apr 22, 2023
socketserver: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer servers #103673
Closed
@bedevere-bot bedevere-bot added the awaiting review label Apr 22, 2023
@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

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jb2170 and gpshead reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Lib/socketserver.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead self-assigned this Apr 23, 2023
gpshead and others added 6 commits April 23, 2023 12:44
@gpshead @sobolevn
style nit: add a missing space.
d634059 
admittedly other existing lines don't always do this. this module is very old code.

Co-authored-by: Nikita Sobolev <[email protected]>
@gpshead
ReSTify the NEWS entry.
a9a91ed
@gpshead
remove an unnecessary word in NEWS
e32b567
@arhadthedev
Add authorship
58073d4
@jb2170
Update Lib/test/test_socketserver.py to use ForkingUnix{Stream,Datagr…
5f61e05 
…am}Server from socketserver
@jb2170
Merge branch 'main' into pythongh-103673
afb4520
@jb2170

jb2170 commented Apr 24, 2023

Copy link
Copy Markdown
Contributor Author

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 reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead approved these changes Apr 24, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 24, 2023
gpshead
gpshead approved these changes Apr 24, 2023
View reviewed changes
gpshead added 2 commits April 24, 2023 14:56
@gpshead
Merge branch 'main' into pythongh-103673
7d0ac34
@gpshead
Add a versionadded 3.12 tag.
ee13a90
@gpshead
gpshead enabled auto-merge (squash) April 24, 2023 22:06
@gpshead
gpshead merged commit d94b3a6 into python:main Apr 24, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label 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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@jb2170 @gpshead @sobolevn @bedevere-bot @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.