thriftrw / thriftrw-python

A Thrift encoding library for Python
MIT License
36 stars 10 forks source link

Check the magnitude of integer types in validate(). #132

Closed TheJakeSchmidt closed 8 years ago

TheJakeSchmidt commented 8 years ago

This partially fixes GitHub issue #131.

This is not a complete solution, as the error message is still opaque, but it's an improvement. Throwing the error at struct creation time instead of at serialization time makes it clearer which struct has a bad field, and makes it easier for TChannel clients to handle, since TChannel doesn't seem to provide a way to catch response serialization exceptions.

HelloGrayson commented 8 years ago

@TheJakeSchmidt mind adding a description to your PR? A quick summary about the motivation and solution should work.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.06%) to 78.569% when pulling f783835f875072f770eb3551bc97de71287a7728 on TheJakeSchmidt:improve-OverflowError-message into 8fa5d67a1f7d9ea3d31612d7ab060c7a8e133b89 on thriftrw:master.

abhinav commented 8 years ago

+1 to Grayson's comment. Please explain the change and link to the relevant GH issue (#131) in the commit message.

Minor comments besides that but the approach looks good.

TheJakeSchmidt commented 8 years ago

Thanks for the reviews! I added a description to the PR and the initial commit, and responded to both comments.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.06%) to 78.45% when pulling c52404133a80518b4490c9368aceee695500fd1f on TheJakeSchmidt:improve-OverflowError-message into 8fa5d67a1f7d9ea3d31612d7ab060c7a8e133b89 on thriftrw:master.

abhinav commented 8 years ago

LGTM.

Please add an entry to the changelog and I'll merge this.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.06%) to 78.45% when pulling ce63d739fc387129a38f065e268d1a74c725a1c8 on TheJakeSchmidt:improve-OverflowError-message into 8fa5d67a1f7d9ea3d31612d7ab060c7a8e133b89 on thriftrw:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.06%) to 78.45% when pulling ce63d739fc387129a38f065e268d1a74c725a1c8 on TheJakeSchmidt:improve-OverflowError-message into 8fa5d67a1f7d9ea3d31612d7ab060c7a8e133b89 on thriftrw:master.