-
Notifications
You must be signed in to change notification settings - Fork 587
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
add status reason to UnsatisfiedAssumption rejections #3830
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.
CI is being nitpicky, but this looks great to me - merge once it's passing!
def __init__(self, *, reason=None): | ||
self.reason = reason |
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.
Apparently we have a test for whether we have a test for all error classes with custom init methods!
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.
test_exceptions_are_picklable
actually caught a bug due to this, which is that UnsatisfiedAssumption
was no longer picklable after making reason
a kwarg. To be honest, reason
seems sensible as a posarg instead, so I turned it into one (which consequently fixed this issue).
I'm not particularly familiar with pickling issues like this, so maybe there is a more obvious solution.
Switched to |
I would hit the merge button here (you did tell me to!), but I changed some internals since, so I'm going to hold off. Free to yell at me if you think that was a bad call 🙂 |
That's all about trusting your judgement, and if you want another review I'm happy to do so. In this case it all looks good, so merging now! |
closes #3827.
Note that what we originally discussed in the issue has the knock-on effect of adding to the events of that test case, which shows up in e.g. statistics mode reporting:
I'd call this desirable, though perhaps a tad confusing as it doesn't state which assume statement was unsatisfied. But I wonder if there was a good reason this hasn't been done yet.
cc @hgoldstein95