Modifies PartialEq for Violation
#208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Changes `PartialEq` for `Violation` to defer to flattened violations for equivalence rather then `Violation`s tree * Modified `flattened_violations` to return sorted vector absed on constraint * Modified `flattened_violations` to not return the violation itself when there are no children
popematt
reviewed
Feb 21, 2024
ion-schema/src/violation.rs
Outdated
| ), | ||
| ], | ||
| ); | ||
| assert_ne!(violation1.violations(), violation2.violations()); |
There was a problem hiding this comment.
question—why are we asserting that this are not equal? This seems like it's not relevant to the test.
There was a problem hiding this comment.
Err: this was part of debugging. I will remove this.
ion-schema/src/violation.rs
Outdated
| use crate::violation::{Violation, ViolationCode}; | ||
|
|
||
| #[test] | ||
| fn violation_equivalence() { |
There was a problem hiding this comment.
Can you add another test case with multiple levels of nesting?
There was a problem hiding this comment.
Sure. I will add that.
There was a problem hiding this comment.
Also, if we're writing tests for eq, we should also include cases where things are not equal and other edge cases.
constraintnot equalcodenot equalmessagenot equalion_pathnot equalflattened_violations()not equalviolationsnot equal, butflattened_violations()equalviolationsempty for bothviolationswith two child violations with the same constraint name
There was a problem hiding this comment.
PartialEq had missing ion_path equivalence check. Added a condition check for that along with more tests.
popematt
reviewed
Feb 22, 2024
popematt
reviewed
Feb 28, 2024
* Adds macros for asserting violation equivalence * Changes `violations` back to a vector instead of `HashSet`
popematt
approved these changes
Mar 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #206:
Description of changes:
This PR works on fixing
PartialEqforViolationand modifiesflattened_violationsreturn sorted violations which can be used for comparison.List of changes:
PartialEqforViolationto defer to flattened violationsfor equivalence rather then
Violations treeflattened_violationsto return sorted vector absed onconstraint
flattened_violationsto not return the violation itselfwhen there are no children
Test:
Adds test for violations equivalence.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.