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

gh-122449: Use constants instead of hard-coded strings in Tools/cases_generator #122448

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Jul 30, 2024

This is a small PR for avoiding using hardcoded token's kind. By the way, I observed something weird: In always_exits of analyzer.py we have

elif tkn.kind == "GOTO" or tkn.kind == "RETURN":
return True
elif tkn.kind == "KEYWORD":
if tkn.text in EXITS:
return True
elif tkn.kind == "IDENTIFIER":

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).

@picnixz picnixz added skip issue skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jul 30, 2024
@picnixz picnixz requested a review from markshannon as a code owner July 30, 2024 11:39
@picnixz picnixz changed the title Use constants instead of hard-coded strings in Tools/cases_generator gh-122449: Use constants instead of hard-coded strings in Tools/cases_generator Jul 30, 2024
@@ -3,6 +3,7 @@
import lexer
import parser
import re
from lexer import *
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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 as lx.XXX.

@picnixz picnixz marked this pull request as draft November 15, 2024 12:48
@picnixz picnixz marked this pull request as ready for review November 15, 2024 13:21
@picnixz picnixz requested a review from sobolevn November 16, 2024 08:58
Copy link
Member

@sobolevn sobolevn left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants