Thriftpy / thriftpy2

Pure python approach of Apache Thrift.
MIT License
572 stars 91 forks source link

Rename socket_timeout to timeout for compatibility #115

Closed aiudirog closed 4 years ago

aiudirog commented 4 years ago

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.

codecov[bot] commented 4 years ago

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%) :arrow_up:

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 commented 4 years ago

Maybe a little test case on this should be cool?

aiudirog commented 4 years ago

@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 commented 4 years ago

Great job πŸ‘ LGTM

aiudirog commented 4 years ago

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

ethe commented 4 years ago

πŸ‘ good job