-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
gh-122449: Use constants instead of hard-coded strings in Tools/cases_generator
#122448
base: main
Are you sure you want to change the base?
Conversation
Tools/cases_generator
Tools/cases_generator
…-kind # Conflicts: # Tools/cases_generator/analyzer.py # Tools/cases_generator/generators_common.py
Tools/cases_generator/analyzer.py
Outdated
@@ -3,6 +3,7 @@ | |||
import lexer | |||
import parser | |||
import re | |||
from lexer import * |
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.
I personally don't like star imports. What do you think about explicit names? Or import lexer
and then lexer.NAME
?
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.
Oh sorry, I should have used a draft PR for now. I actually wanted the star import to see which names are needed and then I'll change them one by one.
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.
I decided to:
- avoid changing the current imports and other usage
- always use
import lexer as lx
when many constants need to be imported. Then, I use them aslx.XXX
.
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.
Generally looks like a good refactoring, but I am not a code owner of this module, so I cannot make the full review.
This is a small PR for avoiding using hardcoded token's kind. By the way, I observed something weird: In
always_exits
ofanalyzer.py
we havecpython/Tools/cases_generator/analyzer.py
Lines 537 to 542 in d27a53f
but a token kind set to "KEYWORD" does not exist (I added the constant but did not use it at all and I don't know how you could create one of that kind). I assume that the test should instead be something like
tkn.kind in lx.keywords.values()
instead but I would like confirmation on that matter.I did not touch the kind of a Node since they are already literals (and mypy would then comply if something is wrong).
Tools/cases_generator
#122449