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

Change identifier replacer to allow for all valid Python identifiers #202

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

marvinvanaalst
Copy link
Contributor

Overview

Change check for valid identifiers from

_IDENTIFIER_PATTERN: ClassVar[re.Pattern] = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")

to using the stdlib Python functions str.isidentifier and keyword.iskeyword

Details

The previous identifier pattern has two problems.
a) It blocks a lot of names as a lot of UTF-8 characters, e.g. α , are valid identifiers
b) It doesn't block invalid keywords as Python names, e.g. def

The stdlib str.isidentifier and keyword.iskeyword functions allow to correctly check for both cases and as a bonus are maintained by the Python folks, so new keyword additions etc. aren't a problem

I replaced the check in the IdentifierReplacer class and added an additional check for the def keyword in the test cases.

As a side note: is it necessary to check for the key to be a valid Python identifier? Since the key comes from parsing an existing Python function it cannot be invalid in the first place

@marvinvanaalst
Copy link
Contributor Author

Oh and optionally I'd change the value error message to something like f"'{v}' is not a valid Python identifier., because when I first read the error message I was a bit confused as to what exactly identifier name was referring to (e.g. LaTeX or Python or something internal to the package)

Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@odashi odashi merged commit c2caf5e into google:main Dec 8, 2023
10 checks passed
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.

2 participants