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.

Sign up for GitHub

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

Jump to bottom

gh-90953: Emit deprecation warnings for ast features deprecated in Python 3.8 #104199

Merged
merged 29 commits into from
May 6, 2023
Merged

gh-90953: Emit deprecation warnings for ast features deprecated in Python 3.8 #104199

merged 29 commits into from
May 6, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented May 5, 2023 •
edited
Loading

This is a continuation of @serhiy-storchaka's PR #31432, but removes the deprecation warnings for ast.ExtSlice and ast.Index, which were only deprecated in Python 3.9 (rationale for excluding them: #31432 (comment)).

📚 Documentation preview 📚: https://cpython-previews--104199.org.readthedocs.build/

@AlexWaygood AlexWaygood added type-feature A feature request or enhancement stdlib Python modules in the Lib dir 3.12 bugs and security fixes labels May 5, 2023
@AlexWaygood AlexWaygood requested a review from isidentical as a code owner May 5, 2023 12:03
@bedevere-bot bedevere-bot mentioned this pull request May 5, 2023
Closed
AlexWaygood
AlexWaygood commented May 5, 2023
View reviewed changes
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
hugovk
hugovk approved these changes May 5, 2023
View reviewed changes
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2022-02-19-14-19-34.bpo-46797.6BXZX4.rst Outdated Show resolved Hide resolved
@AlexWaygood AlexWaygood mentioned this pull request May 5, 2023
Closed
hugovk
hugovk reviewed May 5, 2023
View reviewed changes
Lib/test/test_ast.py
self.assertIsInstance(n, N)
self.assertIsinstance(n, ast.Num)
self.assertNotIsInstance(n, N2)
self.assertNotIsInstance(ast.Num(42), N)
Copy link
Member

Choose a reason for hiding this comment

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

Nice :)

By the way, https://github.com/isidentical/teyit / https://pypi.org/project/teyit/ is a nice tool that can upgrade some of these automatically.

barneygale
barneygale reviewed May 5, 2023
View reviewed changes
Copy link
Contributor

@barneygale barneygale left a comment

Choose a reason for hiding this comment

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

Is it necessary to emit deprecation warnings when the names are accessed on the ast module? Would it not be sufficient to emit warnings when these classes are instantiated (+ isinstance etc)?

Lib/ast.py Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member Author

Is it necessary to emit deprecation warnings when the names are accessed on the ast module? Would it not be sufficient to emit warnings when these classes are instantiated (+ isinstance etc)?

I guess I lean towards keeping them just to ensure maximum visibility. There are edge cases where you might not be "using" the class or calling isinstance() on it, but you do reference it (if type(blah) is ast.Num, etc.). But you are right that we could probably simplify the code somewhat if we only warned on instantiation and isinstance() checks.

@hugovk, do you have any thoughts on this?

@hugovk
Copy link
Member

hugovk commented May 6, 2023

If there are two common ways of accessing something, I'd also lean towards more visibility for both.

Speaking from a position of having dealt with the fallout of removing something without a DeprecationWarning first, just mentioning in docs :)

@AlexWaygood AlexWaygood enabled auto-merge (squash) May 6, 2023 16:20
@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 6, 2023 •
edited
Loading

Thanks very much, both of you!

@AlexWaygood AlexWaygood merged commit 376137f into python:main May 6, 2023
@AlexWaygood AlexWaygood deleted the ast-deprecation-warnings branch May 6, 2023 16:51
jbower-fb pushed a commit to jbower-fb/cpython-jbowerfb that referenced this pull request May 8, 2023
@AlexWaygood @serhiy-storchaka @jbower-fb
pythongh-90953: Emit deprecation warnings for ast features deprecat…
1173ce2 
…ed in Python 3.8 (python#104199)

`ast.Num`, `ast.Str`, `ast.Bytes`, `ast.Ellipsis` and `ast.NameConstant` now all emit deprecation warnings on import, access, instantation or `isinstance()` checks.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@seifertm seifertm mentioned this pull request May 9, 2023
Closed
@iritkatriel
Copy link
Member

@AlexWaygood Is it time to make the PR to remove them now?

@AlexWaygood
Copy link
Member Author

@AlexWaygood Is it time to make the PR to remove them now?

yes

@AlexWaygood AlexWaygood mentioned this pull request May 25, 2024
Closed
@AlexWaygood
Copy link
Member Author

@AlexWaygood Is it time to make the PR to remove them now?

See #119563

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk approved these changes

@barneygale barneygale barneygale approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@isidentical isidentical Awaiting requested review from isidentical isidentical is a code owner

Assignees
No one assigned
Labels
3.12 bugs and security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AlexWaygood @hugovk @iritkatriel @barneygale @bedevere-bot @serhiy-storchaka