Skip to content
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

Eventually exception fix #103

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

bandophahita
Copy link
Contributor

Fixes #102

Copy link
Member

@perrygoy perrygoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few musings, but i think the checklist of things to check for "equality" of an exception is good. 👍

Thanks for the quick fix!

Comment on lines +304 to +311
def test_mention_multiple_errors_once(self, mocked_time, Tester):
mocked_time.time = mock.create_autospec(time.time, side_effect=[1, 1, 1, 100])
mock_question = FakeQuestion()
mock_question.answered_by.return_value = True
mock_question.describe.return_value = "returns bool"

with pytest.raises(DeliveryError) as actual_exception:
Eventually(See(mock_question, IsEqualTo(False))).perform_as(Tester)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure, did this test fail before adding the fix above?

Comment on lines +168 to +174
def same_exception(a: BaseException, b: BaseException) -> bool:
"""compare exceptions to see if they match"""
return (
isinstance(a, type(b))
and (str(a) == str(b))
and (format_tb(a.__traceback__) == format_tb(b.__traceback__))
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some things i'd like to see for this function:

  • The name, maybe isequal_exception? Kinda like isinstance?
  • the parameters, we can do better than a and b. Maybe even first_exc and second_exc?
  • The docstring, i think it can be a little more explicit about what we're doin' here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have mixed feelings about this function hanging out in Eventually, but i can't figure out what exactly makes me feel like i want to avoid that. It being on the outside means we can use it elsewhere if we need to, which is nice, but is this the right place for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you said something. I almost put it into a module called "common" or some such. But YAGNI prevented me. If we ever need the function elsewhere, we can move it then.

@bandophahita bandophahita merged commit e6f618f into ScreenPyHQ:trunk Sep 28, 2023
9 checks passed
@bandophahita bandophahita deleted the eventually_exception_fix branch September 28, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eventually logging multiple occurances of the same exception
2 participants