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

Add support for i8 (alias to byte) #296

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

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.

@@ -399,6 +400,8 @@ def p_simple_base_type(p): # noqa
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.

@@ -399,6 +400,8 @@ def p_simple_base_type(p): # noqa
p[0] = TType.BOOL
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.

None yet

3 participants