-
Notifications
You must be signed in to change notification settings - Fork 51
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
Option to treat exceptions as equivalent #328
Conversation
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.
I know this is draft, so you probably aren't even ready for a review, but I also figure that delaying feedback never helps 😄. This is looking great, and I am excited about it! Some overall notes:
- Append yourself to
contributing.rst
! Most people link to their github, but any self-promotion link you want is fine. precommit -a
hopefully will hopefully help you get past the current CI complaints.- Related, I'd love to hear any stumbling blocks you might have encountered while getting a working dev environment set up!
cbcd268
to
25fbd39
Compare
instead of returning the exception directly. unpack IgnoreAttempt while displaying lint
25fbd39
to
88c5146
Compare
Update diff_behavior.py
687a83d
to
6ebde1e
Compare
crosshair/diff_behavior.py
Outdated
if flexible_equal(args1, args2) and ( | ||
flexible_equal(result1[0], result2[0]) # Compare only the results. | ||
): |
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.
Swap the order of these clauses to fix that strange test_diff_behavior_nan
failure. (flexible_equal(result1[0], result2[0]) and flexible_equal(args1, args2)
)
There's actually a latent bug in the nan comparison thing I just did, but it only gets exposed because you've changed the order of the comparison, and that's enough to make the system pick different paths in the test. I'll fix the root cause separately (and increase the iterations in the test so that we would be able to detect a regression in the future)
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.
Thanks!
to fix nan issue undo lint
caf6ae5
to
2b55f2b
Compare
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.
And CI is clean! Thank you so much for this! I am sort of surprised there's a conflict vs main; let me know if you'd like my help resolving that.
Thanks Phillip! I am also surprised about the conflicts. I tried merging the main into the working branch, hopefully that resolves it? I cannot see a conflict on the PR |
Supposed merge conflicts were having trouble; I ended up squashing and committing in 8ed4356 Thank you @Abhiram98 ! |
For #324