-
Notifications
You must be signed in to change notification settings - Fork 2
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
Union fix #8
Open
anujdas
wants to merge
6
commits into
Saltside:master
Choose a base branch
from
anujdas:wip/union_fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Union fix #8
Commits on May 2, 2018
-
We already need `rake` to work with this gem, and it's capable of basically everything that `make` is; unify build tools to prevent confusion. Move the test thrift files into the test/ directory to avoid clutter, plus add a `clean` target for `rake` that wipes built definitions. Also, add minitest as a dependency, since it’s not built into newer Ruby versions.
Configuration menu - View commit details
-
Copy full SHA for c2b7a00 - Browse repository at this point
Copy the full SHA c2b7a00View commit details -
Fix union and collection validation edge cases
Some things passed/returned to Thrift endpoints are not themselves Thrift objects, e.g., lists, primitives, or nil. These can and should still be validated. Since we're anyway not making use of type hints in the schema and relying on Thrift structs to self-validate, we can just recurse the struct tree and let what can be validated be validated, greatly cleaning up implementation while adding functionality. For example, union validation was broken for a very specific edge case: when a field that was a container around structs is not set, validation produces a highly confusing error message indicating that the unset field is, in fact, not set. Consider the Thrift definition union UnionExample { 1: list<SomeStruct> primary, 2: string alternate, } Setting the `alternate` field caused validation to fail, claiming that the `primary` field was not the set_field. This was due to the validator accessing the unset field, because the code meant to check this case only ran for specific field types. Now it performs correctly regardless of field type.
Configuration menu - View commit details
-
Copy full SHA for de46952 - Browse repository at this point
Copy the full SHA de46952View commit details -
Fix inconsistency in union validation errors
Union validation used to raise a StandardError, which doesn't make sense when the rest of the validation failures cause ProtocolExceptions. Make that uniform.
Configuration menu - View commit details
-
Copy full SHA for f12f9d0 - Browse repository at this point
Copy the full SHA f12f9d0View commit details -
Configuration menu - View commit details
-
Copy full SHA for 6f7cd02 - Browse repository at this point
Copy the full SHA 6f7cd02View commit details -
The library's error changes and new functionality seem like enough to bump the minor version. In particular, union validation now raises a different error (ProtocolException) than it used to (StandardError).
Configuration menu - View commit details
-
Copy full SHA for ee60561 - Browse repository at this point
Copy the full SHA ee60561View commit details
Commits on May 4, 2018
-
Validator#validate is an instance method; reflect this in the docs.
Configuration menu - View commit details
-
Copy full SHA for 4817332 - Browse repository at this point
Copy the full SHA 4817332View commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.