Skip to content
This repository was archived by the owner on Dec 10, 2018. It is now read-only.

Conversation

@stigsb
Copy link

@stigsb stigsb commented May 13, 2017

Apache Thrift has deprecated the byte type in favor of i8 in recent versions, this patch makes thriftpy support both.

@stigsb
Copy link
Author

stigsb commented May 27, 2017

Hi guys,
Any chance we could have this fix reviewed? (it seems Travis failed because of an unrelated issue)

Copy link
Contributor

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me.

p[0] = TType.BOOL
if p[1] == 'byte':
p[0] = TType.BYTE
if p[1] == 'i8':
Copy link
Contributor

@jparise jparise Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These conditions should probably become elif so we don't continuously reevaluate p[1] after we find a match.

... but that would be better down in a separate change.

if p[1] == 'byte':
p[0] = TType.BYTE
if p[1] == 'i8':
p[0] = TType.BYTE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not good to reuse TType.BYTE, as I8 you defined in p_simple_base_type is useless now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants