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

Fix reading bool value using compact protocol #153

Merged
merged 3 commits into from
Aug 9, 2017
Merged

Fix reading bool value using compact protocol #153

merged 3 commits into from
Aug 9, 2017

Conversation

reisub
Copy link
Contributor

@reisub reisub commented Aug 9, 2017

We came across an issue in deserializing a bool value using compact protocol - the value always comes out false.

This PR modifies the test so that it catches this bug and fixes it.

@msftclas
Copy link

msftclas commented Aug 9, 2017

@reisub,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #153 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #153      +/-   ##
============================================
+ Coverage     65.45%   65.48%   +0.02%     
- Complexity      931      932       +1     
============================================
  Files            88       88              
  Lines          4806     4806              
  Branches        561      561              
============================================
+ Hits           3146     3147       +1     
  Misses         1423     1423              
+ Partials        237      236       -1
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/thrifty/protocol/CompactProtocol.java 78.24% <100%> (ø) 63 <0> (ø) ⬇️
...in/java/com/microsoft/thrifty/schema/Location.java 78.04% <0%> (+2.43%) 16% <0%> (+1%) ⬆️

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 85ff3c7...8840ab3. Read the comment docs.

@msftclas
Copy link

msftclas commented Aug 9, 2017

@reisub, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@benjamin-bader
Copy link
Collaborator

Excellent catch - thanks for the fix! Would you mind including the thrift change you made to Xtruct as well, so we don't lose it if/when we ever regenerate it?

@reisub
Copy link
Contributor Author

reisub commented Aug 9, 2017

@benjamin-bader done in 8840ab3, though I'm not sure which exact file you wanted changed since there are none in the thrift-runtime module.

@benjamin-bader
Copy link
Collaborator

Perfect. There's no one file, sadly - we seem to have overlooked automating generating this file, probably because it is done so infrequently. I think this would only be the second version.

I'm comfortable calling the integration-test file the "source of truth" here.

@benjamin-bader benjamin-bader merged commit 39b99da into microsoft:master Aug 9, 2017
@benjamin-bader
Copy link
Collaborator

Merged - thanks again!

@reisub reisub deleted the fix/compact-read-bool branch August 10, 2017 10:50
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.

None yet

4 participants