-
-
Notifications
You must be signed in to change notification settings - Fork 417
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
Clean up tree templates implementation to reduce mypy errors #1091
Clean up tree templates implementation to reduce mypy errors #1091
Conversation
class _ReplaceVars(Transformer): | ||
def __init__(self, conf, vars): | ||
def __init__(self, conf: TemplateConf, vars: Mapping[str, Tree[str]]) -> None: | ||
super().__init__() |
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.
Since Transformer
has an __init__()
, it is best to call it in case it does something more important in the future.
8d3c16e
to
a8f6da5
Compare
a8f6da5
to
ac98ba1
Compare
@@ -129,7 +136,7 @@ def search(self, tree: TreeOrCode): | |||
if res: | |||
yield subtree, res | |||
|
|||
def apply_vars(self, vars: Mapping[str, Tree]): | |||
def apply_vars(self, vars: Mapping[str, Tree[str]]) -> Tree[str]: |
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.
This annotation for vars
was here originally but conflicts with the annotations I've added based on how the code was written. The key issue is that TemplateConf._match_tree_template()
returns a dictionary where the value could be a str
or Tree[str]
. This then ripples up here because translate()
uses this result as the vars
argument. This could be a case that the annotation on vars
was not completely accurate. Or, since it was one of the few annotations used initially, that it is the intended API. If it is the second, then there would need to a second pass on the implementation to have everything line up.
Mostly type annotation changes, but also includes a few code tweaks.
Based on how
Tree
was used in the file, I'm fairly certain that the intended use case isTree[str]
. However, it is possible that this can be mage even more precise withParseType
(and alias forTree[Token]
).Resolves