Skip to content

New directive +IGNORE_WARNINGS to capture and silence warnings - #84

Merged
astrofrog merged 5 commits into
scientific-python:masterfrom
astrofrog:ignore-warnings
Nov 12, 2019
Merged

astrofrog merged 5 commits into
scientific-python:masterfrom
astrofrog:ignore-warnings

Conversation

@astrofrog

Copy link
Copy Markdown
Contributor

There are cases where lines in a doctest will emit a warning but we don't necessarily want to show it in the output or have the warning end up being counted by pytest. This new directive will make sure all warnings on that line will be silenced.

@vanyakosmos - feel free to review this if you'd like, since you are very familiar with this plugin! :)

@pllim - you might also be interested in this :)

@pllim pllim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I needed this. Thank you!

Hmm. Looking at http://doc.pytest.org/en/latest/doctest.html#using-doctest-options , I wonder if IGNORE_WARNING_DETAIL is more consistent with the closest counterpart that it has with what pytest ships. But I have no preference.

@pllim

pllim commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

p.s. Do we still have to support Python 2? ImportError: cannot import name indent

@bsipocz

bsipocz commented Nov 12, 2019 •

Copy link
Copy Markdown
Member

@astrofrog - could you also turn this into a directive, too? so we could seamlessly add to rst code snippets.

Comment thread tests/test_doctestplus.py
>>> warnings.warn('A warning occurred', UserWarning) # doctest: +IGNORE_WARNINGS
""")
reprec = testdir.inline_run(p, "--doctest-plus", "--doctest-rst",
"--text-file-format=rst", "-W error")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flag should work without -W error, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was to make sure that the warning triggers a failure in the tests - for older pytest versions this isn't the case.

import sys
import warnings
from textwrap import indent

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as @pllim said, we should either drop support for python 2 or suffer. In py2 indent can be added using text wrapper class: https://docs.python.org/2.7/library/textwrap.html#textwrap.TextWrapper

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a quick workaround for now but agree that we'll want to drop Python 2 shortly.

@astrofrog

Copy link
Copy Markdown
Contributor Author

Hmm. Looking at http://doc.pytest.org/en/latest/doctest.html#using-doctest-options , I wonder if IGNORE_WARNING_DETAIL is more consistent with the closest counterpart that it has with what pytest ships. But I have no preference.

This is different - for an exception it makes sure that the traceback of the exception doesn't match but that the final error message does. Here we really just want to pretend like the warning doesn't exist.

could you also turn this into a directive, too?

I'll investigate!

@astrofrog

astrofrog commented Nov 12, 2019 •

Copy link
Copy Markdown
Contributor Author

@bsipocz - so in principle yet we could add a directive for this, and I started working on it, but then I realized that I'm not sure how we deal with the situation where multiple directives apply (what if we need 'requires' and 'ignore-warnings'?), and what syntax to use in that case (this hasn't been relevant until now). We also don't have directives for all options. So for now I'd rather we just merged this as-is and then in future if we think there would be a good use case for a directive, we can cross that bridge?

@astrofrog

Copy link
Copy Markdown
Contributor Author

Since this was approved, I'll go ahead and merge this and will open an issue to continue the discussion about directives.

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.

4 participants