Skip to content
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

Handle family passed as keyword argument #75

Merged
merged 4 commits into from Dec 20, 2021

Conversation

emontnemery
Copy link
Contributor

@emontnemery emontnemery commented Sep 1, 2021

Handle family passed as keyword argument to socket constructor

@emontnemery
Copy link
Contributor Author

emontnemery commented Oct 5, 2021

@miketheman can this get merged, or are there any concerns?

Instead of this:

def __new__(cls, *args, **kwargs):

We could perhaps do:

def __new__(cls, family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None)

@miketheman
Copy link
Owner

Hi @emontnemery !

I'm lacking context as to why this is an improvement on the existing behavior, as well as any tests to validate the change works as intended.

@miketheman miketheman added enhancement New feature or request more-info-needed Further information is requested labels Oct 6, 2021
@emontnemery
Copy link
Contributor Author

The current code throws if the socket is created with its parameters passed as keywords instead of as positional arguments, this PR fixes that by handling both cases.

I've added testcases where socket.socket() is called with keyword arguments or no arguments.

pytest_socket.py Outdated
@@ -86,7 +86,10 @@ class GuardedSocket(socket.socket):
""" socket guard to disable socket creation (from pytest-socket) """
def __new__(cls, *args, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should be changed to def __new__(cls, family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None) instead, then we can simply check family, instead of handling both positional and keyword arguments cases.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting - maybe this should be more compatible with the defaults of the underlying class? https://github.com/python/cpython/blob/59435eea08d30796174552c0ca03c59b41adf8a5/Lib/socket.py#L220

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, updated to mimic the CPython implementation, i.e.:

def __new__(cls, family=-1, type=-1, proto=-1, fileno=None)

@codeclimate
Copy link

codeclimate bot commented Dec 13, 2021

Code Climate has analyzed commit fc0ba71 and detected 0 issues on this pull request.

View more on Code Climate.

@miketheman miketheman removed the more-info-needed Further information is requested label Dec 20, 2021
@miketheman miketheman added this to the 0.50.0 milestone Dec 20, 2021
@miketheman miketheman self-assigned this Dec 20, 2021
@miketheman miketheman merged commit 6da69ca into miketheman:main Dec 20, 2021
@miketheman
Copy link
Owner

Thanks for working through this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants