Skip to content
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

Context manager to ensure no warnings are issued #9745

Closed
h-vetinari opened this issue Mar 9, 2022 · 4 comments
Closed

Context manager to ensure no warnings are issued #9745

h-vetinari opened this issue Mar 9, 2022 · 4 comments
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@h-vetinari
Copy link

This is a direct continuation of #9404, where the API discussion would have taken too long to further hold up 7.0.0, and which was therefore "closed" by a documentation fix.

While I cannot reproduce the wealth of discussion from that other issue, there was the following summary by @The-Compiler:

My conclusions for this:

  • pytest.warns(None) might have been understood in three different ways actually:
    1. Ensuring that no warnings at all are emitted
    2. Suppressing warnings (surprisingly common)
    3. Ensuring that any warning is emitted
  • I think @Zac-HD was absolutely right about deprecating this usage!
  • We should probably have a good way to do all of those things, and point them out appropriately in the deprecation message
  • Conditionally passing a warning class or None seems very rare, so I wouldn't want to block 7.0 to find a solution to that problem.

In addition to explicitly enforcing the absence of warnings (and the other use-cases presented above), I also think it would be a very worthwhile to have a (higher-level?) API that eases switching between different scenarios. For example, I recently found myself writing the following:

        if some_condition:
            with pytest.warns(
                DeprecationWarning,
                match=r"Non-integer arguments are currently being cast to",
            ):
                result = special.comb(N, k, exact=True, legacy=legacy,
                                      repetition=repetition)
        else:
            result = special.comb(N, k, exact=True, legacy=legacy,
                                  repetition=repetition)

In particular, the calculation is the same in both cases and it takes a lot of ceremony to capture the only relevant difference (the warning). In pseudo-code, the intent boils down to (ignoring the match= clause for now and reusing an idea from the last thread):

        warnings = [DeprecationWarning] if some_condition else []
        with pytest.event(exceptions=[], warnings=warnings):
            result = special.comb(N, k, exact=True, legacy=legacy,
                                  repetition=repetition)

Note this idea did not gain any traction at the time, but I don't have a better suggestion for now (as changing behaviour of pytest.warn() or pytest.except() was considered infeasible due to huge spread; I also find it unattractive to switch the context-manager itself, e.g. to a putative pytest.does_not_warn()).

@Zac-HD
Copy link
Member

Zac-HD commented Mar 16, 2022

My position continues to be:

  1. I stand by the current, post-deprecation, semantics of pytest.warns() ("emits any warning", shorthand for pytest.warns(Warning))
  2. If we support asserting no warnings, I think the best API is with pytest.does_not_warn(category=Warning):. The implementation should be a very thin wrapper over with warnings.catch_warnings(): warnings.filterwarnings("error", category=category); .... IMO the best argument against this is that it only saves a single line of code compared to using the standard library, which we could explain in our docs.
  3. If we support suppressing warnings, I'd spell that with pytest.ignore_warnings(category=Warning):; implementation the same with "ignore" instead of "error".

I'd also argue against providing super-powerful combinations - libraries are best when they provide clean and composable pieces. If people have complex needs - they have access to all of Python! (including ExitStack for combining context managers) Let them write a helper function that suits their project, instead of making pytest harder to learn🙂

@Zac-HD Zac-HD added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Mar 20, 2022
@Zac-HD
Copy link
Member

Zac-HD commented Mar 21, 2022

If the standard library warnings module was less verbose, I'd just tell people to use it directly:

# Status quo / implementation of proposed `with pytest.does_not_warn():`
with warnings.catch_warnings():
    warnings.simplefilter("error")
    # your code here...

So I think it's worth proposing upstream that catch_warnings() should accept the arguments for simplefilter(), and forward them through for an initial call. This is easy to implement (or backport), a pleasantly concise API, and encourages users to learn the standard library warnings interface instead of avoiding it. Instead of new APIs, we'd just document that:

  • with does_not_warn(): is spelled with warnings.catch_warnings(action="error"):
  • with ignore_warnings(): is spelled with warnings.catch_warnings(action="ignore"):
  • and you can customize them with the category argument for particular warning types.
  • For more details, see the warnings module documentation in the standard library.

@Zac-HD
Copy link
Member

Zac-HD commented Apr 24, 2022

Since I've gotten the above proposal into CPython 3.11, I'm closing this issue. For now people can use with catch_warnings() and the explicit call to simplefilter(); from 3.11 onwards you can use one of the following:

with warnings.catch_warnings(action="ignore"):
    warnings.warn("This will be ignored")

with warnings.catch_warnings(action="error"):
    warnings.warn("This will be an exception")

with pytest.warns():
    pass  
# This will be an error because a warning was expected

@Zac-HD Zac-HD closed this as completed Apr 24, 2022
@nicoddemus
Copy link
Member

That's awesome, thanks @Zac-HD!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests

3 participants