Skip to content

Conversation

@djkirkham
Copy link
Contributor

It's confusing that assertGivesWarning can be used to assert that no warning is raised. I've refactored the code into separate methods.

@djkirkham djkirkham requested a review from pp-mo October 26, 2017 09:28
Copy link
Member

@pelson pelson left a comment

Choose a reason for hiding this comment

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

Happy conceptually. The golden problem of naming remains... 😄

# byte type.
cube = self._make_cube('>i1')
with self.assertGivesWarning(r'\(fill\|mask\)', expect_warning=False):
with self.assertDoesNotGiveWarning(r'\(fill\|mask\)'):
Copy link
Member

Choose a reason for hiding this comment

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

rename? assertNoWarningsMatching?

Copy link
Member

Choose a reason for hiding this comment

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

@djkirkham suggests: assertDoesNotGiveWarningRegexp

Copy link
Member

Choose a reason for hiding this comment

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

assertDoesNotGiveWarningMatchingRegexp

Copy link
Member

Choose a reason for hiding this comment

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

assertNoWarningsMatchingRegexp

if expr.search(message))

@contextlib.contextmanager
def assertGivesWarning(self, expected_regexp=''):
Copy link
Member

Choose a reason for hiding this comment

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

rename? assertWarningsMatching?

Copy link
Member

Choose a reason for hiding this comment

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

@djkirkham suggests: assertGivesWarningRegexp

Copy link
Member

Choose a reason for hiding this comment

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

Or even assertGivesWarningMatchingRegexp

@pp-mo
Copy link
Member

pp-mo commented Oct 26, 2017

On consideration, I don't like the original name.
But it shouldn't need to be as unwieldy as some of the recent suggestions.
Anyone for "AssertWarnsRegexp" and "AssertNoWarningsRegexp" ?

@pelson
Copy link
Member

pelson commented Oct 26, 2017

Some genuine test failures...

@djkirkham djkirkham force-pushed the assertDoesNotGiveWarning branch from d190be9 to 4f8b2d8 Compare October 26, 2017 16:11
@pelson
Copy link
Member

pelson commented Oct 27, 2017

The failing test issue has been raised in #2875.

@pelson pelson merged commit c93132b into SciTools:master Oct 27, 2017
@QuLogic QuLogic added this to the v2.0.0 milestone Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants