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

Rename socket_timeout to timeout for compatibility #115

Merged
merged 1 commit into from
Jan 19, 2020

Conversation

aiudirog
Copy link
Collaborator

@aiudirog aiudirog commented Jan 18, 2020

The standard make_client() uses the timeout argument to specify the socket timeout whereas the asyncio make_client() calls it socket_timeout. Since AIOHappyBase handles both, it leads to awkward dispatch issue where I have to rewrite the argument for one.

This PR deprecates socket_timeout in favor of just timeout so that the async make_client() matches the standard one. I chose this and not the other way around so it would have the least impact on existing code, even though the socket_timeout name is more descriptive and probably better.

@aiudirog aiudirog requested a review from ethe January 18, 2020 19:01
@codecov
Copy link

codecov bot commented Jan 18, 2020

Codecov Report

Merging #115 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage    79.7%   79.71%   +0.01%     
==========================================
  Files          43       43              
  Lines        3902     3905       +3     
==========================================
+ Hits         3110     3113       +3     
  Misses        792      792
Impacted Files Coverage Δ
thriftpy2/contrib/aio/rpc.py 84.44% <100%> (+1.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82d5a79...5bd4ef4. Read the comment docs.

@aisk
Copy link
Member

aisk commented Jan 19, 2020

Maybe a little test case on this should be cool?

@aiudirog
Copy link
Collaborator Author

@aisk I mocked out TAsyncSocket and validated that the arguments are handled as expected. I also squashed the commits since I had some small hiccups with flake8 and Py3.5.

@aisk
Copy link
Member

aisk commented Jan 19, 2020

Great job 👍 LGTM

@aiudirog
Copy link
Collaborator Author

I'll go ahead and merge this in. Thanks!

@aiudirog aiudirog merged commit b78720e into Thriftpy:master Jan 19, 2020
@ethe
Copy link
Member

ethe commented Jan 20, 2020

👍 good job

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

Successfully merging this pull request may close these issues.

3 participants