-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-129027: Raise DeprecationWarning for sys._clear_type_cache #129043
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
Conversation
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.
sys._clear_type_cache
is used in other tests as well which will now raise a DeprecationWarning
. I think we should suppress the warnings in the relevant tests (or maybe switch to sys._clear_internal_caches()
? though that might make backporting tests to 3.12 more difficult since the function was added in 3.13)
This will also require a news entry :) |
Misc/NEWS.d/next/Library/2025-01-21-11-48-19.gh-issue-129027.w0vxzZ.rst
Outdated
Show resolved
Hide resolved
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.
Would you also like to add a What's New entry as @picnixz suggested?
Misc/NEWS.d/next/Library/2025-01-21-11-48-19.gh-issue-129027.w0vxzZ.rst
Outdated
Show resolved
Hide resolved
@tomasr8 Thanks 👍🏾 . Please review again? |
The What's New is still missing (it's something that you need to add in Doc/whatsnew/3.14.rst I think). The NEWS (blurb) entry is only for the full changelog, but deprecation notices are also communicated there so that they have better visibility. You can put this change in the Pending Removal section (as we are now emitting a warning) |
I'll review it in a few minutes, but I have some comments on some wording. And I think we already mentioned to avoid force pushing, so please do so in the future. Avoid rebasing to merge upstream and prefer using the update button instead. Or if it becomes too messy, just create a new branch to be sure you don't trigger unnecessary review requests |
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.
After commenting the files, I think you can create a wrapper around _clear_type_cache
that suppresses its deprecation warning use instead of suppressing the warning around the entire test. If other deprecation warnings appear we want them to be printed.
Misc/NEWS.d/next/Library/2025-01-21-11-48-19.gh-issue-129027.w0vxzZ.rst
Outdated
Show resolved
Hide resolved
Lib/test/test_type_cache.py
Outdated
@@ -15,11 +18,13 @@ | |||
type_assign_specific_version_unsafe = _testinternalcapi.type_assign_specific_version_unsafe | |||
type_assign_version = _testcapi.type_assign_version | |||
type_modified = _testcapi.type_modified | |||
ignore_deprecation = warnings_helper.ignore_warnings(category=DeprecationWarning) |
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 should be a dummy decorator if _clear_type_cache
is None.
Lib/test/test_cmd_line.py
Outdated
def test_python_dump_refs(self): | ||
code = 'import sys; sys._clear_type_cache()' | ||
rc, out, err = assert_python_ok('-c', code, PYTHONDUMPREFS='1') | ||
self.assertEqual(rc, 0) | ||
|
||
@unittest.skipUnless(sysconfig.get_config_var('Py_TRACE_REFS'), "Requires --with-trace-refs build option") | ||
@warnings_helper.ignore_warnings(category=DeprecationWarning) |
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.
Can we use this one only when doing assert_python_ok
?
Lib/test/test_cmd_line.py
Outdated
@@ -932,12 +932,14 @@ def test_python_asyncio_debug(self): | |||
self.assertIn(b'True', out) | |||
|
|||
@unittest.skipUnless(sysconfig.get_config_var('Py_TRACE_REFS'), "Requires --with-trace-refs build option") | |||
@warnings_helper.ignore_warnings(category=DeprecationWarning) |
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.
Ditto
Lib/test/test_type_cache.py
Outdated
@@ -61,6 +66,7 @@ class C: | |||
self.assertNotEqual(type_get_version(C), 0) | |||
self.assertNotEqual(type_get_version(C), c_ver) | |||
|
|||
@ignore_deprecation |
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.
Can we only protect the _clear_type_cache
call instead of the entire test?
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
…0vxzZ.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz Please review again. |
Lib/test/test_type_cache.py
Outdated
with warnings.catch_warnings(): | ||
warnings.simplefilter("ignore", DeprecationWarning) |
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.
Maybe you can wrap clear_type_cache
with a waraning filter:
def clear_type_cache():
with warnings.catch_warnings():
...
_clear_type_cache()
@picnixz Done. |
@srinivasreddy There are some conflicts with main now, could you fix them please? |
@tomasr8 Done. |
DeprecationWarning
forsys._clear_type_cache
#129027