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

Binary support #164

Merged
merged 47 commits into from
Apr 12, 2021
Merged

Binary support #164

merged 47 commits into from
Apr 12, 2021

Conversation

JonnoFTW
Copy link
Contributor

@JonnoFTW JonnoFTW commented Feb 10, 2021

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.

# Conflicts:
#	thriftpy2/protocol/apache_json.py
…ated calls to `self.trans.read(1)` takes a long time with large requests

(cherry picked from commit 89c4cb1)
(cherry picked from commit 1088e68)
(cherry picked from commit d87d5cc)
(cherry picked from commit 1d5c395)
(cherry picked from commit 4dcff99)
(cherry picked from commit bb4ea29)
(cherry picked from commit 6d1b8c9)
(cherry picked from commit b33ac62)
(cherry picked from commit 2fef901)
(cherry picked from commit 9031ed8)
(cherry picked from commit 8b97de9)
(cherry picked from commit 506f52e)
(cherry picked from commit 19c833b)
(cherry picked from commit 03431f8)
(cherry picked from commit a9ca53f)
(cherry picked from commit 1963f85)
(cherry picked from commit 23ab765)
(cherry picked from commit 624d763)
(cherry picked from commit 1ef62fe)
Made asyncio binary protocol backward compatible with apache thrift

(cherry picked from commit dc1bab2)
(cherry picked from commit 6b35a72)
(cherry picked from commit a60739a)
(cherry picked from commit e1e9a53)
(cherry picked from commit 5a4fcbb)
@codecov
Copy link

codecov bot commented Feb 10, 2021

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%) ⬇️
thriftpy2/contrib/aio/protocol/compact.py 60.17% <50.00%> (-0.82%) ⬇️
thriftpy2/protocol/binary.py 89.25% <76.47%> (-1.06%) ⬇️
thriftpy2/protocol/compact.py 84.54% <91.30%> (+2.27%) ⬆️
thriftpy2/protocol/apache_json.py 97.59% <97.59%> (ø)
thriftpy2/protocol/__init__.py 87.50% <100.00%> (+0.83%) ⬆️
thriftpy2/protocol/json.py 96.74% <100.00%> (+13.98%) ⬆️
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 JonnoFTW changed the title Binary support [WIP] Binary support Feb 10, 2021
@JonnoFTW
Copy link
Contributor Author

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

@ethe
Copy link
Member

ethe commented Feb 19, 2021

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

@JonnoFTW JonnoFTW changed the title [WIP] Binary support Binary support Feb 19, 2021
@JonnoFTW
Copy link
Contributor Author

@ethe I have fixed the title.

@ethe
Copy link
Member

ethe commented Apr 12, 2021

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
Copy link
Member

ethe commented Apr 12, 2021

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
Copy link
Contributor Author

JonnoFTW commented Apr 12, 2021

@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.

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.

2 participants