Add support for union, binary fields, and empty structs #14

Merged
merged 5 commits into from Jun 18, 2014

Conversation

Projects
None yet
2 participants
@dan-blanchard
Contributor

dan-blanchard commented Jun 18, 2014

This PR adds support for unions, binary fields, empty structs, and semicolons at the ends of lines within structs, with the goal of allowing someone to load the Apache Storm thrift file.

Some notes about implementation:

  • unions are just loaded up as structs for now. In the future we might want to modify _compat.init_func_generator to enforce the restriction that you are only supposed to set one of the fields of a union.
  • binary fields are just special cases of string, so when receiving string fields in the binary protocol, we try to decode as utf-8 first, but if that fails we just return the bytes.
  • There were a few places where the grammar had OneOrMore instead of ZeroOrMore, which prevented declaring empty structs. I've fixed that.
  • The Thrift IDL specifies that you can use semicolons or commas as list separators in some places, so I've added support for semicolons.

I'll finish adding the test data tomorrow; I just wanted to trigger a travis build to make sure everything was working so far.

dan-blanchard added some commits Jun 18, 2014

Add support for union, binary fields, and empty structs.
`unions` are just loaded up as `structs` for now. In the future we might
want to modify `_compat.init_func_generator` to enforce the restriction
that you are only supposed to set one of the fields of a union.

`binary` fields are just special cases of `string`, so when receiving
`string` fields in the binary protocol, we try to decode as `utf-8`
first, but if that fails we just return the bytes.

There were a few places where the grammar had `OneOrMore` instead of
`ZeroOrMore`, which prevented declaring empty structs. I've fixed that.
@dan-blanchard

This comment has been minimized.

Show comment
Hide comment
@dan-blanchard

dan-blanchard Jun 18, 2014

Contributor

This is supposed to address #13.

Contributor

dan-blanchard commented Jun 18, 2014

This is supposed to address #13.

@dan-blanchard

This comment has been minimized.

Show comment
Hide comment
@dan-blanchard

dan-blanchard Jun 18, 2014

Contributor

Alright, now I've added unit tests that use the Storm thrift file and make sure we can load empty structs, unions, things with semicolons, and the binary field type.

I'm only seeing one unit test failure and that's with test_rpc on pypy, and I have no idea why that's happening, since I didn't change anything related to that.

Contributor

dan-blanchard commented Jun 18, 2014

Alright, now I've added unit tests that use the Storm thrift file and make sure we can load empty structs, unions, things with semicolons, and the binary field type.

I'm only seeing one unit test failure and that's with test_rpc on pypy, and I have no idea why that's happening, since I didn't change anything related to that.

@dan-blanchard

This comment has been minimized.

Show comment
Hide comment
@dan-blanchard

dan-blanchard Jun 18, 2014

Contributor

Actually, I guess that pypy failure was just a onetime fluke. Everything passes now.

Contributor

dan-blanchard commented Jun 18, 2014

Actually, I guess that pypy failure was just a onetime fluke. Everything passes now.

lxyu added a commit that referenced this pull request Jun 18, 2014

Merge pull request #14 from dan-blanchard/support_storm
Add support for union, binary fields, and empty structs

@lxyu lxyu merged commit f17906f into eleme:master Jun 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@dan-blanchard dan-blanchard deleted the dan-blanchard:support_storm branch Jun 18, 2014

@lxyu

This comment has been minimized.

Show comment
Hide comment
@lxyu

lxyu Jun 18, 2014

Contributor

Thank you very much for your excellent PR!

Contributor

lxyu commented Jun 18, 2014

Thank you very much for your excellent PR!

@dan-blanchard

This comment has been minimized.

Show comment
Hide comment
@dan-blanchard

dan-blanchard Jun 18, 2014

Contributor

Thank you for making a nicer version of thrift for Python so that I didn't have to. 😄

Contributor

dan-blanchard commented Jun 18, 2014

Thank you for making a nicer version of thrift for Python so that I didn't have to. 😄

lxyu added a commit that referenced this pull request Jun 18, 2014

add BINARY TType to cybinary protocol
The cybinary protocol uses local cdef TTypes, which was missed in #14
@lxyu

This comment has been minimized.

Show comment
Hide comment
@lxyu

lxyu Jun 18, 2014

Contributor

FYI, I just did a v0.1.3 release. You may use pypi for convenient installation now.

Contributor

lxyu commented Jun 18, 2014

FYI, I just did a v0.1.3 release. You may use pypi for convenient installation now.

@dan-blanchard

This comment has been minimized.

Show comment
Hide comment
@dan-blanchard

dan-blanchard Jun 18, 2014

Contributor

Awesome. Thanks!

Contributor

dan-blanchard commented Jun 18, 2014

Awesome. Thanks!

@dan-blanchard dan-blanchard referenced this pull request in Parsely/streamparse Apr 23, 2015

Closed

Move away from Clojure #136

@pyup-bot pyup-bot referenced this pull request in scieloorg/opac_proc Aug 18, 2016

Closed

Initial Update #10

@pyup-bot pyup-bot referenced this pull request in scieloorg/scielo-manager Aug 26, 2016

Closed

Pin thriftpy to latest version 0.3.9 #1314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment