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.

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

issue#6305 Replace deprecated ast.NameConstant usage and add tests #6306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lizdenhup
Copy link

@lizdenhup lizdenhup commented Apr 3, 2024

Problem

Within misc.py there is a usage of ast.NameConstant. ast.NameConstant will be deprecated beginning with Python 3.14.x so we want to remove it.

Solution

  • Replace ast.NameConstant with ast.Constant within misc.py
  • Add tests to test situations where the generator returns an ast.Constant with a value, or with a None value

Validation

  • I added tests. The tests pass.

Fixes #6305

@@ -2,6 +2,7 @@
import warnings
from functools import partial
from unittest import mock
import ast
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be used in tests directly.

Copy link
Author

Choose a reason for hiding this comment

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

Hiya! Good call out -- I amended my commit to use magic mock. Let me know what you think. Cheers.

@@ -12,6 +12,13 @@
def _indentation_error(*args, **kwargs):
raise IndentationError()

def create_ast_return(value=None):
return (
unittest.mock.MagicMock(name='ast.Return', value=None)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm my point is that I expect this test to test real code that produces specific ast nodes when parsed.

Copy link
Member

Choose a reason for hiding this comment

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

(hpefully that makes sense)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that makes sense. I will push a new change tomorrow. Thank you for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ast.NameConstant is deprecated and will be removed in Python 3.14; use ast.Constant instead
2 participants