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

deprecate non-cm raises,warns&deprecated call + add paramspec #13241

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Feb 20, 2025

deprecation

Deprecates the legacy callable forms for pytest.raises, pytest.warns and pytest.deprecated_call, and replaces all instances in the code base of them aside from code specifically for testing them.

paramspec

pytest.raises, warns & deprecated_call previously typed *args and **kwargs as Any in the legacy callable form, so this did not raise errors:

def foo(x: int) -> None:
    raise ValueError
raises(ValueError, foo, None)

but now it will give call-overload.

It also makes it possible to pass func as a kwarg, which the type hints previously showed as possible, but it didn't work.

It's possible that func (and the expected type?) should be pos-only, as this looks quite weird:

raises(1, 2, kwarg1=3, func=my_func, kwarg2=4, expected_exception=ValueError)

but if somebody is dynamically generating parameters to send to raises then we probably shouldn't ban it needlessly; and we can't make func pos-only without making expected_exception pos-only, and that could break backwards compatibility.

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

Noticed while working on #13192

@jakkdl jakkdl added the topic: typing type-annotation issue label Feb 20, 2025
@jakkdl jakkdl requested review from nicoddemus and Zac-HD February 20, 2025 10:11
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Feb 20, 2025
@RonnyPfannschmidt
Copy link
Member

I believe the key issue is that back when the api was initially created, neither typing nor POS only arguments where a thing

In hindsight, both func and expected exceptions should be POS only arguments

Some kind of deprecating overload would be needed (no cluesif typing supports anyof that)

I'm thinking whether we should just downright deprecate the call form

Back in python 2.5 times when we had no with statement there only was a call form and a exec form (multiline stringto eval)

Now that soon python 3.8 support is dropped we perhaps ought to just go context only and plan accordingly

@jakkdl
Copy link
Member Author

jakkdl commented Feb 20, 2025

I believe the key issue is that back when the api was initially created, neither typing nor POS only arguments where a thing

In hindsight, both func and expected exceptions should be POS only arguments

I could make them pos-only for RaisesExc and RaisesGroup in #13192 to start with, so we only need to do deprecation on pytest.raises itself.

Some kind of deprecating overload would be needed (no cluesif typing supports anyof that)

@warnings.deprecated does indeed allow for deprecating individual overloads.

I'm thinking whether we should just downright deprecate the call form

Back in python 2.5 times when we had no with statement there only was a call form and a exec form (multiline stringto eval)

Now that soon python 3.8 support is dropped we perhaps ought to just go context only and plan accordingly

the docs does say

This form was the original pytest.raises() API, developed before the with statement was added to the Python language. Nowadays, this form is rarely used, with the context-manager form (using with) being considered more readable. Nonetheless, this form is fully supported and not deprecated in any way.

(emphasis mine) which kind of sounds like us wanting to support it indefinitely, but I have never seen the form used outside of the pytest repo itself, so I think we should be plenty fine deprecating it.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 21, 2025

"well, let's just deprecate them then!"
...

thankfully I had LLM generate a regex that did a lot of the replacements, but, uh, fun times.

I don't think there's much to gain from deprecating pytest.raises(expected_exception=ValueError)?

TODO:

  • Changelog
  • update PR title & description (lol)
  • update docs

@jakkdl jakkdl changed the title add paramspec to non-cm raises deprecate non-cm raises,warns&deprecated call + add paramspec Feb 24, 2025
@jakkdl
Copy link
Member Author

jakkdl commented Feb 24, 2025

the other stuff causing <100% diff coverage is me editing currently-uncovered code

jakkdl and others added 3 commits February 25, 2025 15:33
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @jakkdl for tackling this.

Left comments mostly related to the docs.

However I wonder if we really should deprecate this functionality?

  • We never said it would deprecated.
  • Changing to the context manager is non-trivial to do, which will cause a lot of friction in older codebases.
  • The code to support the legacy form seems pretty minimal to me, so we gain little by removing the support.

So my vote on this is -0, as I think the disruption/friction outweighs the small benefit of removing the supporting code, but if everybody else thinks this is worthwhile, then I'm OK with it too.

Comment on lines +33 to +39
# deprecated callable form

excinfo = pytest.raises(ValueError, int, "hello")
ret1 = pytest.warns(DeprecationWarning, my_warns, "a", "b", "c")
ret2 = pytest.deprecated_call(my_warns, "d", "e", "f")

# context-manager form
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# deprecated callable form
excinfo = pytest.raises(ValueError, int, "hello")
ret1 = pytest.warns(DeprecationWarning, my_warns, "a", "b", "c")
ret2 = pytest.deprecated_call(my_warns, "d", "e", "f")
# context-manager form
# Deprecated form, using a callable + arguments:
excinfo = pytest.raises(ValueError, int, "hello")
ret1 = pytest.warns(DeprecationWarning, my_warns, "a", "b", "c")
ret2 = pytest.deprecated_call(my_warns, "d", "e", "f")
# The calls above can be upgraded to the context-manager form:

@@ -194,30 +194,6 @@ exception at a specific level; exceptions contained directly in the top
assert not excinfo.group_contains(RuntimeError, depth=2)
assert not excinfo.group_contains(TypeError, depth=1)

Alternate form (legacy)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should remove this section yet -- the callable form has been deprecated, but not yet removed.

We should keep it, just adding a .. warning:: that it is now considered deprecated and will be removed in a future version.

@@ -14,6 +14,37 @@ Deprecated Features
Below is a complete list of all pytest features which are considered deprecated. Using those features will issue
:class:`~pytest.PytestWarning` or subclasses, which can be filtered using :ref:`standard warning filters <warnings>`.

legacy callable form of :func:`raises <pytest.raises>`, :func:`warns <pytest.warns>` and :func:`deprecated_call <pytest.deprecated_call>`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
legacy callable form of :func:`raises <pytest.raises>`, :func:`warns <pytest.warns>` and :func:`deprecated_call <pytest.deprecated_call>`
Legacy callable form of :func:`raises <pytest.raises>`, :func:`warns <pytest.warns>` and :func:`deprecated_call <pytest.deprecated_call>`


CALLABLE_RAISES = PytestDeprecationWarning(
"The callable form of pytest.raises is deprecated.\n"
"Use `with pytest.raises(...):` instead."
Copy link
Member

@nicoddemus nicoddemus Mar 1, 2025

Choose a reason for hiding this comment

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

Let's add a link to the deprecations doc which shows the rationale and examples on how to change the code.

Suggested change
"Use `with pytest.raises(...):` instead."
"Use `with pytest.raises(...):` instead.\n"
"See https://docs.pytest.org/en/stable/reference/deprecations.html#legacy-callable-form-of-raises-warns-and-deprecated-call"

Same for the messages below.


It can also be used by passing a function and ``*args`` and ``**kwargs``,
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as before: given we are deprecating but not removing, we should keep the docs for the legacy form, with a warning that it has been deprecated.

return warns(
(DeprecationWarning, PendingDeprecationWarning, FutureWarning), *args, **kwargs
)
# potential QoL: allow `with deprecated_call:` - i.e. no parens
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# potential QoL: allow `with deprecated_call:` - i.e. no parens
# Potential QoL: allow `with deprecated_call:` - i.e. no parens.

Comment on lines +25 to +35
if sys.version_info >= (3, 13):
from warnings import deprecated as deprecated
elif TYPE_CHECKING:
from typing_extensions import deprecated as deprecated
else:

def deprecated(reason: str = "") -> object:
def decorator(func: object) -> object:
return func

return decorator
Copy link
Member

@nicoddemus nicoddemus Mar 1, 2025

Choose a reason for hiding this comment

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

warnings.deprecated will emit a warning at runtime in addition to be used by type checkers, but not for Python <= 3.12, which IIUC is a problem then.

Seems like the only solution is to always generate the runtime warning ourselves, and only rely on the deprecated decorator when TYPE_CHECKING?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, since this PR does manually raise the warning everywhere we should be getting double warnings on 3.13 ... and either the tests are not catching it or the second one is getting suppressed.

@jakkdl
Copy link
Member Author

jakkdl commented Mar 3, 2025

  • The code to support the legacy form seems pretty minimal to me, so we gain little by removing the support.

It's not a lot of supporting code at its face, but it took me a while to figure out gc troubles in 13192#2c8cd and raises could entirely be removed in place of a from ... export RaisesExc as raises after #13192.

It's also a question of whether to keep it maintained with RaisesExc - e.g. should it now accept the check parameter? If so, that will break code:

def my_fun(check):
    ...

def test_my_fun():
    pytest.raises(my_fun, check=5)

if we wanted to fully embrace it then maybe RaisesGroup should have a callable form, but that'd be kinda silly.

  • Changing to the context manager is non-trivial to do, which will cause a lot of friction in older codebases.

it'd be nice to get a sense of how prevalent it is, I've personally never encountered it outside of the pytest code base itself.

So my vote on this is -0, as I think the disruption/friction outweighs the small benefit of removing the supporting code, but if everybody else thinks this is worthwhile, then I'm OK with it too.

I agree that it's not a slam-dunk, and if we deprecate it we should make sure to have a long deprecation cycle. Heck, we could try deprecating it and if there's complaints we consider reversing. (futuredeprecationwarning? and/or a message explicitly saying so)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR topic: typing type-annotation issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants