-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-96348: Deprecate the 3-arg signature of coroutine.throw, generator.throw and agen.athrow #96428
gh-96348: Deprecate the 3-arg signature of coroutine.throw, generator.throw and agen.athrow #96428
Conversation
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
There is a problem: raise Eg: Lines 154 to 155 in 22ed523
|
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.
One nit. Too bad about the failing tests, let's brainstorm what to do about them:
- We could just fix them, replacing
.throw(A, B, C)
with.throw(B)
. - We could add
filterwarnings()
calls to the offending tests (I don't recall how to do that but there are probable plenty of examples). - We could report a
SilentDeprecationWarning
, then gradually fix the tests, and then later change it into aDeprecationWarning
. - We could first fix the tests, in batches, and then land this PR.
Whoops, there's no such thing. Maybe it once existed, but it doesn't now, so ignore this option. :-) |
Let's estimate the problem size, by finding with regex There are 51 results in 12 files.
I prefer fixing them in this PR, for the fix need to be verified by raising an error. Otherwise the batch fix might miss something.
|
Okay, make sure you consider this. Also, some of the hits may be tests for this very feature, those should not be "fixed" but you may have to add a |
Co-authored-by: Guido van Rossum <[email protected]>
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
I run However, there are still some 1. Generator's doc testI think we should filter the cpython/Lib/test/test_generators.py Lines 2116 to 2130 in 0cd33e1
2. Lib
|
with self: | |
callable_obj(*args, **kwargs) |
The output is like:
/../cpython/Lib/unittest/case.py:237: DeprecationWarning: the (type, val, tb) exception representationis deprecated, and may be removed in a future version of Python.
callable_obj(*args, **kwargs)
I am not sure what callable_obj
does here, and maybe filtering this warning requires a change in standard library of python?
Misc/NEWS.d/next/Core and Builtins/2022-08-31-18-46-13.gh-issue-96348.xzCoTP.rst
Outdated
Show resolved
Hide resolved
Look at the code in Lib/unittest/case.py. This is part of the assertRaises mechanism. The callable_obj and the args passed to it are defined by the test, so this should be dealt with in the particular test and not here in the test framework. |
The test is this one. It's testing the types of the triplet, so we need to keep it and suppress the deprecation warning.
|
…e-96348.xzCoTP.rst Co-authored-by: Irit Katriel <[email protected]>
Okay, handled the remaining warnings. Tested with those scripts: ./python -m test -j16 > test.log; cat test.log | grep "DeprecationWarning: the (type, val, tb) exception representation is deprecated"
# Come out with nothing
./python -Werror -m unittest -v test.test_asyncio.test_futures.PyFutureTests.test_future_iter_throw test.test_generators
# OK Besides, should I squash all commits into one, when all the reviews are done? |
Don’t squash please! It makes review harder. We will squash upon merge. |
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.
This is a significant change, it should be properly documented in 3.12 What's New.
Misc/NEWS.d/next/Core and Builtins/2022-08-31-18-46-13.gh-issue-96348.xzCoTP.rst
Outdated
Show resolved
Hide resolved
…e-96348.xzCoTP.rst Co-authored-by: Irit Katriel <[email protected]>
…an supress them." This reverts commit 8738273.
assert that deprecation warning is emitted
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 8b701d6 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
It seems that my tests exposed some problem - there are 3 versions of futures, and only one of them is emitting the deprecation warning? |
Looking at the code, if futures._CFuture is fixed (made to raise the deprecation warning) then that will fix CSubFuture as well. |
So I think we need one more deprecation warning from FutureIter_throw in |
The warning is added. But I still feel a little dizzy about it - How do you know that we should add a test in test_futures.py? Is python future implemented by coroutine object or async generator?
|
@kumaraditya303 How does it look now? (I'm a little dizzy by now as well). |
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.
LGTM
You only need to add deprecation warning in cpython/Lib/asyncio/futures.py Lines 290 to 298 in 9a11ed8
|
@@ -2996,6 +2996,11 @@ generators, coroutines do not directly support iteration. | |||
above. If the exception is not caught in the coroutine, it propagates | |||
back to the caller. | |||
|
|||
.. versionchanged:: 3.12 | |||
|
|||
The second signature \(type\[, value\[, traceback\]\]\) is deprecated and |
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.
For future PRs, not that code should generally be marked up like
The second signature `(type[, value[, traceback]])` is deprecated and
Add an deprecation warning, when
nargs > 1
inObjects/genobject.c:gen_throw
.gen.throw(typ, val, tb)
#96348