fix(types): some typing updates #1149
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
henryiii
force-pushed
the
henryiii/fix/types
branch
from
0fe029d
to
66c22d0
Compare
|
@erezsh Need input on the best path to take to solve the last remaining mypy error. |
|
I think all the return types should just be |
| - name: Lint with mypy | ||
| run: mypy -p lark || true | ||
| run: pipx run tox -e type |
There was a problem hiding this comment.
I love that GHA supports pipx as a first-class package manager!
|
|
||
| [testenv] | ||
| whitelist_externals = git | ||
| deps = | ||
| -rtest-requirements.txt | ||
| passenv = | ||
| TERM |
There was a problem hiding this comment.
This allows mypy to detect that color is supported by the running terminal (well, assuming it is), and causes it to print colored output instead of uncolored output.
There was a problem hiding this comment.
Nox doesn't filter environment variables, but Tox does. TERM shouldn't ever be filtered, since tools might check it to see what you are running in (like mypy does).
|
Looks good to me! @MegaIng Might be nice to have another pair of eyes. |
lark/tree_templates.py
Outdated
| @@ -55,7 +55,7 @@ def _get_tree(self, template: TreeOrCode) -> Tree[str]: | |||
| def __call__(self, template: Tree[str]) -> 'Template': | |||
| return Template(template, conf=self) | |||
|
|
|||
| def _match_tree_template(self, template: TreeOrCode, tree: TreeOrCode) -> Optional[Dict[str, TreeOrCode]]: | |||
| def _match_tree_template(self, template: TreeOrCode, tree: Tree) -> Optional[Dict[str, Tree]]: | |||
There was a problem hiding this comment.
The annotation for the argument tree isn't quite correct based on the rest of the function, or the test template == tree in the if isinstance(template, str) case below is not needed.
There was a problem hiding this comment.
I actually also think that this function will happily correctly return a Dict[str, Token]. It's not really TreeOrCode, it's TreeOrToken.
There was a problem hiding this comment.
Actually, the problem wasn't with these functions returning TreeOrCode (although it's a bit of a misnomer), it's the problem of everything else not accepting TreeOrToken as an input in the type signature.
There was a problem hiding this comment.
Yeah, only the return types needed to change, the argument should have stayed TreeOrCode. I missed it.
It's actually not Token, because it accepts regular strings. But tokens are included in it.
There was a problem hiding this comment.
@henryiii I think the only thing needed it to change back tree: Tree into tree: TreeOrCode
There was a problem hiding this comment.
@MegaIng Hmm you may have a point!
There was a problem hiding this comment.
I think that would mean this has to change instead:
def apply_vars(self, vars: Mapping[str, Tree[str]]) -> Tree[str]:into
def apply_vars(self, vars: Mapping[str, TreeOrCode]) -> TreeOrCode:And same with the _ReplaceVars class.
There was a problem hiding this comment.
(@henryiii I don't mind fixing it up myself, if you prefer)
There was a problem hiding this comment.
I think it might need to be a bound TypeVar in at least a few places, since the input matches the output. translate does seem to require a tree, at least by the time it starts operating on .data and .children. Translate is also missing a return annotation.
I don't mind fixing it up myself, if you prefer
That might be best, I'll revert my change here, then you can edit the PR (or I can type ignore it, then you can follow up, whatever you like).
There was a problem hiding this comment.
No problem, fixed it in #1150.
Sorry for the mixup! It's definitely tricky code, and I wasn't the one who originally added types to it. And in fact, it caught a real edge case!
henryiii
force-pushed
the
henryiii/fix/types
branch
from
d35c29a
to
66c22d0
Compare
|
Fixed |
|
@henryiii You can merge my branch here, and then I'll merge this PR. Or you can close this PR and I'll just merge the new one. Whatever works for you. |
|
You can merge yours, happy either way and not at a keyboard ATM. |
|
No problem. Thanks for the contribution! |
Added mypy configuration, and fixing all but one of the typing issues.
The remaining one I'm not sure about:
I'm using
toxsince that's already set up. I personally prefernox; better defaults, no custom language, explains what it's doing while it's doing it. I've also updated the CI config. And for mypy, I preferpre-commit(the tool); easy to update pins, ultra fast to run due to excellent caching, single place for all style checks & fixers.