-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: main
Are you sure you want to change the base?
Conversation
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 |
I could make them pos-only for RaisesExc and RaisesGroup in #13192 to start with, so we only need to do deprecation on
@warnings.deprecated does indeed allow for deprecating individual overloads.
(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. |
"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 TODO:
|
the other stuff causing <100% diff coverage is me editing currently-uncovered code |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
There was a problem hiding this 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.
src/_pytest/deprecated.py
Outdated
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, @deprecated
is only being used together with @overload
in this PR; so it is solely used for TYPE_CHECKING
. I'll clarify that with comments/docstring in case somebody wants to use it more in the future
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 It's also a question of whether to keep it maintained with def my_fun(check):
...
def test_my_fun():
pytest.raises(my_fun, check=5) if we wanted to fully embrace it then maybe
it'd be nice to get a sense of how prevalent it is, I've personally never encountered it outside of the
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. ( |
perhaps before deprecating we should see to have tools like ruff supply a code fix and/or have a ast-grep receipt to do the code fix the code change itself is not that hard, but its a massive friction without a tool that does it |
I like that, I'll go open some issues. edit: |
Let's see if the future warning triggers any issues for users I'm on board however python warning is quite some unexpected fun sometimes |
hm, while the default python filter will hide If we do want to hold off on even that then I should split this PR and we can merge the test changes + paramspec on their own |
Let's put it in for now We can put it on defer if there's a problematic impact most recent user's shouldn't be affected |
codecov diff complaints are solely from me changing callable |
deprecation
Deprecates the legacy callable forms for
pytest.raises
,pytest.warns
andpytest.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
asAny
in the legacy callable form, so this did not raise errors: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:but if somebody is dynamically generating parameters to send to
raises
then we probably shouldn't ban it needlessly; and we can't makefunc
pos-only without makingexpected_exception
pos-only, and that could break backwards compatibility.changelog
folder, with a name like<ISSUE NUMBER>.<TYPE>.rst
. See changelog/README.rst for details.Noticed while working on #13192