Skip to content

Let user have more control over deprecation warning to exception - #6223

Merged
bsipocz merged 2 commits into
astropy:masterfrom
pllim:deprec-cfg
Jun 16, 2017
Merged

bsipocz merged 2 commits into
astropy:masterfrom
pllim:deprec-cfg

Conversation

@pllim

@pllim pllim commented Jun 15, 2017 •

Copy link
Copy Markdown
Member

Fix #5917

@bsipocz , is this what you had in mind? Do we need a change log? I also did not set a milestone as I don't think it is critical for v2.0.

Travis tests are on my fork -- https://travis-ci.org/pllim/astropy/builds/243388792 (I cancelled the one here to relieve queue)

@bsipocz

bsipocz commented Jun 15, 2017

Copy link
Copy Markdown
Member

This looks good to me. Would be nice, but I'm not sure how we can test this.

Also, this will definitely need a changelog under astropy.tests as this is a feature for affiliates, and they will need to know about it. Maybe adding to the narrative docs is an option, too. I'm adding the milestone, but as you said it's not critical just something nice to have.

@bsipocz bsipocz added this to the v2.0.0 milestone Jun 15, 2017

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a changelog is needed, but other than that this looks good to me

@pllim

pllim commented Jun 16, 2017

Copy link
Copy Markdown
Member Author

I tested this manually like as follows. First, I have a dummy module:

# filename: ztmp1.py
import warnings

warnings.warn('Oh no!', DeprecationWarning)


def myfunc():
    warnings.warn('Bozo', DeprecationWarning)


def badfunc():
    warnings.warn('Foo', DeprecationWarning)

This session demonstrates how things work by default for module import error:

>>> from astropy.tests.helper import enable_deprecations_as_exceptions, treat_deprecations_as_exceptions
>>> enable_deprecations_as_exceptions()
>>> treat_deprecations_as_exceptions()
>>> import ztmp1
# Some traceback
DeprecationWarning: Oh no!

This session demonstrate how things work with user configuration to bypass module import error but retain default function-level error:

>>> from astropy.tests.helper import enable_deprecations_as_exceptions, treat_deprecations_as_exceptions
>>> enable_deprecations_as_exceptions(modules_to_ignore_on_import=['ztmp1']))
>>> treat_deprecations_as_exceptions()
>>> import ztmp1  # No error on import now
>>> ztmp1.myfunc()  # Default function-level error
# Some traceback
DeprecationWarning: Bozo

This session demonstrate how things work with user configuration to bypass both module import and function-level errors (the latter, only for Python 3.5 or later):

>>> from astropy.tests.helper import enable_deprecations_as_exceptions, treat_deprecations_as_exceptions
>>> enable_deprecations_as_exceptions(
...     modules_to_ignore_on_import=['ztmp1'], warnings_to_ignore_by_pyver={(3, 5): ['Bozo']})
>>> treat_deprecations_as_exceptions()
>>> import ztmp1  # No error on import
>>> ztmp1.myfunc() # Still no error
>>> ztmp1.badfunc()  # Just to show it still works on others
# Some traceback
DeprecationWarning: Foo

I don't know how to turn this into a regression test suite, as I have to do each of those in a separate (fresh) Python session. I am not sure if I want to mess with the warning registry within a regression test; it could ripple through other modules.

pllim added a commit to pllim/package-template that referenced this pull request Jun 16, 2017
pllim added a commit to pllim/package-template that referenced this pull request Jun 16, 2017
@pllim

pllim commented Jun 16, 2017

Copy link
Copy Markdown
Member Author

Change log added. Given the queue, I didn't bother to redo the CI even though I rebased when I added the change log. But if you want, I can squash and force push to make CI run one more time; Just let me know.

As for narrative doc, it would be more obvious over at the package template? See astropy/package-template#259 .

pllim added a commit to pllim/package-template that referenced this pull request Jun 16, 2017
pllim added a commit to pllim/package-template that referenced this pull request Jun 16, 2017
@bsipocz

bsipocz commented Jun 16, 2017

Copy link
Copy Markdown
Member

@pllim - I think this is good enough for now, if someone has a cunning plan for testing it then it can be added in a follow-up PR.

@bsipocz
bsipocz merged commit 3afa751 into astropy:master Jun 16, 2017
@pllim

pllim commented Jun 16, 2017

Copy link
Copy Markdown
Member Author

Thanks! I briefly thought about using mock but seems overkill for this... 😅

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.

2 participants