| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Sorry, something went wrong.
|
There is a problem: raise DeprecationWarning will break many lib tests, since they use the (type, val, tb) exception representation. Eg: Lines 154 to 155 in 22ed523 |
Sorry, something went wrong.
There was a problem hiding this comment.
One nit. Too bad about the failing tests, let's brainstorm what to do about them:
Sorry, something went wrong.
Whoops, there's no such thing. Maybe it once existed, but it doesn't now, so ignore this option. :-) |
Sorry, something went wrong.
Let's estimate the problem size, by finding with regex throw\(.*,.*,.*\) 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.
filterwarnings() call might be a safety valve, if I am not quite confident about how some tests work. |
Sorry, something went wrong.
|
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 with self.warns() on those. |
Sorry, something went wrong.
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Sorry, something went wrong.
|
I run ./python -m test,and fixed clear cases, changed them to .throw(val) form. However, there are still some DeprecationWarnings need to be handle. As @iritkatriel said in comment of 96348, some of the tests need to stay. 1. Generator's doc testI think we should filter the DeprecationWarning here, for it's testing exactly the deprecated API. cpython/Lib/test/test_generators.py Lines 2116 to 2130 in 0cd33e1 2. Lib unittest.caseIt happens here: Lines 236 to 237 in 0cd33e1 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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
The test is this one. It's testing the types of the triplet, so we need to keep it and suppress the deprecation warning. def test_future_iter_throw(self):
fut = self._new_future(loop=self.loop)
fi = iter(fut)
self.assertRaises(TypeError, fi.throw,
Exception, Exception("elephant"), 32)
self.assertRaises(TypeError, fi.throw,
Exception("elephant"), Exception("elephant"))
self.assertRaises(TypeError, fi.throw, list)
|
Sorry, something went wrong.
…e-96348.xzCoTP.rst Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
|
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
# OKBesides, should I squash all commits into one, when all the reviews are done? |
Sorry, something went wrong.
|
Don’t squash please! It makes review harder. We will squash upon merge. |
Sorry, something went wrong.
There was a problem hiding this comment.
This is a significant change, it should be properly documented in 3.12 What's New.
Sorry, something went wrong.
…e-96348.xzCoTP.rst Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
…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. |
Sorry, something went wrong.
|
It seems that my tests exposed some problem - there are 3 versions of futures, and only one of them is emitting the deprecation warning? |
Sorry, something went wrong.
|
Looking at the code, if futures._CFuture is fixed (made to raise the deprecation warning) then that will fix CSubFuture as well. |
Sorry, something went wrong.
|
So I think we need one more deprecation warning from FutureIter_throw in |
Sorry, something went wrong.
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?
|
Sorry, something went wrong.
|
@kumaraditya303 How does it look now? (I'm a little dizzy by now as well). |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
You only need to add deprecation warning in _CFuture since it is implemented in C. The pure Python versions uses regular generators so they will automatically emit warning since warning is added to generator.throw. cpython/Lib/asyncio/futures.py Lines 290 to 298 in 9a11ed8 |
Sorry, something went wrong.
|
|
||
| .. versionchanged:: 3.12 | ||
|
|
||
| The second signature \(type\[, value\[, traceback\]\]\) is deprecated and |
There was a problem hiding this comment.
For future PRs, not that code should generally be marked up like
The second signature `(type[, value[, traceback]])` is deprecated and
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Add an deprecation warning, when nargs > 1 in Objects/genobject.c:gen_throw.