- Notifications
You must be signed in to change notification settings - Fork259
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
check_unique refactor#667
base:pfb/unique-checking
Are you sure you want to change the base?
check_unique refactor#667
Conversation
3d11ec4 to34bc78fCompare…more succinctly without nearly as much repetition.
…t show the `NodeId` and `Graphs`.
34bc78f to7510aecCompareOne downside I see to this approach is that in some cases it forces the node names in the assertion to be out of order compared to the graph, which makes it slightly harder to match the two up. I think we could get the best of both worlds with a macro along the lines of let[a, j, b1, b2, c1, c2, d2] =unique_flags([a, j, b1, b2, c1, c2, d2]);assert!(a && !j && !b1 && !b2 && !c1 && !c2 && d2); |
See#668. That solves this issue I think by putting the expected |
| assert!(!info(&pdg, b3).unique); | ||
| assert!(!info(&pdg, c1).unique); | ||
| assert!(!info(&pdg, c2).unique); | ||
| check_unique(&pdg, &[], &[a, b1, b2, b3, c1, c2]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
having these two arguments really doesn't make clear that you're checking for non-uniqueness here, in addition to the function name. i suggest having one function for each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, that might be better. Might not be worth changing, though, given#668.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see comment
This wouldn't give good error messages, though. |
This whole PR is superseded by the approach in#668, right? |
Yeah, pretty much. |
Refactored all the unique and non-unique
assert!ions intocheck_unique, which is much more concise and avoids the ton of repetition there was previously. It also adds more context to the error message when it fails.