-
Notifications
You must be signed in to change notification settings - Fork 260
Simplify interface for testing of warnings. #345
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
Conversation
else: | ||
from nose.tools import (assert_equal, assert_not_equal, | ||
assert_true, assert_false, assert_raises) | ||
|
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.
@matthew-brett Any idea if try...catch
is still necessary? I took the liberty to blow it away. I couldn't tell from the commits (5+ years ago) where this was being used. I didn't see nt
being used anywhere in my cursory search, and tests pass...
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.
This is so that people can still import nibabel if they don't have nose installed.
Oops - mixed in with the array class refactoring? |
…_warnings. Deprecate ErrorWarnings, IgnoreWarnings, and catch_warn_reset; nibabel.checkwarns.
Everything's set, except one test failure (checking warnings from the |
|
||
def test_ignore_and_error_warnings(): | ||
with clear_and_catch_warnings() as w: | ||
from .. import checkwarns |
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'm guessing what is happening here is that nose has previously imported checkwarns
as part of test discovery for the while test-suite, and therefore there is no Future warning, because the import has already happened.
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.
By the way - I do see the error if I run the full test-suite locally, but not if I run the individual test.
I changed the offending test to an I looked over the coverage change, and I didn't see any obvious gaps (as well as I could from the reports available). I believe this is ready to go, and to focus on #329 , then back to GIFTI / CIFTI (which is what this is all really about!) |
Yes, that's fine, thanks for doing that. |
MRG: Simplify interface for testing of warnings. This is a fix for #343 New interface: suppress_warnings, clear_and_catch_warnings, and error_warnings. Deprecate ErrorWarnings, IgnoreWarnings, and catch_warn_reset; nibabel.checkwarns.
MRG: Simplify interface for testing of warnings. This is a fix for nipy#343 New interface: suppress_warnings, clear_and_catch_warnings, and error_warnings. Deprecate ErrorWarnings, IgnoreWarnings, and catch_warn_reset; nibabel.checkwarns.
This is a fix for #343
I haven't added tests for the deprecation, but I can do that. I am just feeling lazy 😅