Thriftpy / thriftpy2

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

Binary support #164

Closed JonnoFTW closed 3 years ago

JonnoFTW commented 3 years ago

Added binary support per #156

I've added some more tests to cover more cases surrounding binary encodings. If you pass a string to a binary field, it will come out as a bytes type on the other end.

The work here builds off #163 , so that will probably need to be merged before this one.

codecov[bot] commented 3 years ago

Codecov Report

Merging #164 (80264d7) into master (02924c7) will increase coverage by 0.94%. The diff coverage is 89.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   83.37%   84.31%   +0.94%     
==========================================
  Files          43       44       +1     
  Lines        3939     4164     +225     
==========================================
+ Hits         3284     3511     +227     
+ Misses        655      653       -2     
Impacted Files Coverage Δ
thriftpy2/contrib/aio/rpc.py 84.44% <ø> (ø)
thriftpy2/contrib/aio/protocol/binary.py 55.73% <21.42%> (-2.07%) :arrow_down:
thriftpy2/contrib/aio/protocol/compact.py 60.17% <50.00%> (-0.82%) :arrow_down:
thriftpy2/protocol/binary.py 89.25% <76.47%> (-1.06%) :arrow_down:
thriftpy2/protocol/compact.py 84.54% <91.30%> (+2.27%) :arrow_up:
thriftpy2/protocol/apache_json.py 97.59% <97.59%> (ø)
thriftpy2/protocol/__init__.py 87.50% <100.00%> (+0.83%) :arrow_up:
thriftpy2/protocol/json.py 96.74% <100.00%> (+13.98%) :arrow_up:
thriftpy2/thrift.py 90.39% <100.00%> (ø)
... and 3 more

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 02924c7...80264d7. Read the comment docs.

JonnoFTW commented 3 years ago

@ethe are you able to look at this at some point?

ethe commented 3 years ago

I will take a look at it, could you please remove [WIP] in the title?

JonnoFTW commented 3 years ago

@ethe I have fixed the title.

ethe commented 3 years ago

I have a question, does it break the compatibility? If the user defines a binary type and treats it as a string before, what happened after this commit merged?

ethe commented 3 years ago

Maybe we should consider that only turn on this feature in JSON protocol and do not modify others, or turn off it as default in other protocols.

JonnoFTW commented 3 years ago

@ethe please check tests/test_all_protocols_binary_field.py for a test case on how binary fields are treated. I think the best behaviour would be that the type specified in the thrift file is what you get when deserializing.

If a string is passed to a binary field, when deserialised you always get a binary. Conversely, if a binary is passed to a string field, it should be stringified (if possible) when deserializing.