-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
_socket.py: fix for systems where AI_NUMERICSERV is not defined #3135
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3135 +/- ##
=======================================
Coverage 99.62% 99.62%
=======================================
Files 122 122
Lines 18357 18358 +1
Branches 1221 1221
=======================================
+ Hits 18289 18290 +1
Misses 47 47
Partials 21 21
|
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.
Looks good, but needs newsfragment (that is why continuous integration is failing). newsfragments
folder's README file has more information.
@CoolCat467 That one CI which fails is unrelated, right? |
Docs are failing because your newsfragment is using markdown not rST. Try doing It's a bit confusing because the logs show two warnings, but the one you should care about is:
I don't think the other one makes it fail, though I may be wrong. |
641c0df
to
36a3de8
Compare
36a3de8
to
5260922
Compare
# AI_NUMERICSERV may be missing on some older platforms, so use it when available. | ||
_NUMERIC_ONLY = _stdlib_socket.AI_NUMERICHOST | ||
_NUMERIC_ONLY |= getattr(_stdlib_socket, "AI_NUMERICSERV", 0) |
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 think you need to be a bit more specific here, otherwise future people that stumble on this comment will have to do a bunch of research to figure out why/when it might be missing. And as I said in the issue, perhaps even a note on why it's needed despite being deprecated since a decade+
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.
Maybe just linking the issue will work? That has info.
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.
@barracuda156 could you fix this? I think this is the only problem before merging.
(well, barring rejecting this whole change on basis of it being something we cannot test for a system we could not support. IMO this change is fine but @jakkdl didn't respond to the response you left on the issue.)
Rationale: on some platforms
AI_NUMERICSERV
may not be defined. In particular, old versions of macOS, possibly some other. While affected users are probably few, it is a trivial fix, which makestrio
usable (for example,yewtube
app works then).Credits to @CoolCat467
Closes: #3133