Skip to content

Simplify the testing infrastructure for checking warnings #343

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

Closed
bcipolli opened this issue Sep 1, 2015 · 5 comments
Closed

Simplify the testing infrastructure for checking warnings #343

bcipolli opened this issue Sep 1, 2015 · 5 comments

Comments

@bcipolli
Copy link
Contributor

bcipolli commented Sep 1, 2015

In implementing #329, I've hit the need to check warnings. I believe nibabel has two different codepaths for manipulating warnings:

  • nibabel.checkwarns - has classes ErrorWarnings and IgnoreWarnings
  • nibabel.testing - has classes suppress_warnings and catch_warn_reset

I'd like to combine these into a single place, with a single methodology. I believe creating a CheckWarnings class, that mirrors catch_warn_reset, and eliminating the warning classes from nibabel.testing, may be the best?

@matthew-brett any advice? I'd like to resolve this one before completing #329 , if that's cool. I have time to do it today.

@matthew-brett
Copy link
Member

Sorry - I was on a plane flying across the Atlantic yesterday...

Yes, it certainly seems like a good idea to clean these up.

I did an extensive refactor of these things for numpy - here : numpy/numpy#5682 - maybe best to use a copy of that? It had a heavy review.

We should be a little careful not to break anyone else's code with the API change.

@bcipolli
Copy link
Contributor Author

bcipolli commented Sep 2, 2015

👍 thx

@bcipolli
Copy link
Contributor Author

bcipolli commented Sep 2, 2015

OK, so given this, I am thinking to:

  • Deprecate ErrorWarnings, IgnoreWarnings, and catch_warn_reset; also deprecate nibabel.checkwarns.
  • New interface will be suppress_warnings (essentially IgnoreWarnings), clear_and_catch_warnings (copy from numpy), and error_warnings (essentially ErrorWarnings).

Any comments on this plan are welcome.

@matthew-brett
Copy link
Member

Sounds like a good plan. suppress_warnings and error_warnings could inherit from clear_and_catch_warnings too, to allow for modules where the warning has already been raised.

matthew-brett added a commit that referenced this issue Sep 3, 2015
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.
@bcipolli
Copy link
Contributor Author

This was fixed in #345

grlee77 pushed a commit to grlee77/nibabel that referenced this issue Mar 15, 2016
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.
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

No branches or pull requests

2 participants