[mypy] Improve types in _grammar.py #1002
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1002 +/- ##
==========================================
- Coverage 62.73% 61.24% -1.50%
==========================================
Files 62 62
Lines 4396 4412 +16
==========================================
- Hits 2758 2702 -56
- Misses 1638 1710 +72 ☔ View full report in Codecov by Sentry. |
| @@ -494,37 +447,31 @@ def commit_point(value, hidden=False): | |||
| raise NotImplementedError("commit_point is not implemented (may remove in the future)") | |||
|
|
|||
|
|
|||
| def as_regular_grammar(value): | |||
There was a problem hiding this comment.
Had to move this lower when I added type annotations.
| self.values = [v for v in values if not isinstance(v, Null)] | ||
| super().__init__(capture_name=None) | ||
| # wrap raw strings | ||
| converted_values = [string(v) if isinstance(v, (str, bytes)) else v for v in values] |
There was a problem hiding this comment.
TBH, I'd prefer to have Select and Join be strict in their accepted types, and have all the type conversions done by select() and join().
There was a problem hiding this comment.
Totally agreed. Bit of a (related, I think) side note...
How would you feel about making Join a generic type with one argument, bounded above by Function?
Then we can make a String class that subclasses Join[Bytes], where we can have some nice semantics in join, e.g. join(*args: String) -> String. Might be able to do something similar with Select. With the right type annotations, we might be able to get the type-checker to recognize errors like selecting over RawFunctions. Am I making any sense? (I can throw together a little POC if not...)
| if name is not None: | ||
| name = "__LIST_APPEND:" + name | ||
| else: | ||
| raise ValueError("list_append requires a name") |
There was a problem hiding this comment.
New little bit of logic here
| @@ -322,9 +329,8 @@ def __add__(self, other): | |||
| return other | |||
There was a problem hiding this comment.
I know this is already merged -- just looking over it though. Thanks for doing this..!
Do we want to check if other is a str here and upcast the return type via string(other)?
| stack = [ | ||
| (grammar, None, None) | ||
| ] # Stack stores tuples of (node, parent_node, child_index) | ||
| stack = [(grammar, None, None)] # Stack stores tuples of (node, parent_node, child_index) |
There was a problem hiding this comment.
Add a type annotation that matches the comment?
| @@ -919,14 +869,23 @@ def str_to_grammar(value: str): | |||
| return partial_grammar | |||
|
|
|||
|
|
|||
| def _is_string_literal(node: GrammarFunction): | |||
| def _is_string_literal(node: ComposableGrammar) -> bool: | |||
There was a problem hiding this comment.
This would essentially be the TypeGuard for the proposed String = Join[Bytes] class
Working to improve the type annotations in
_grammar.py:GrammarFunction.... at least make them less badComposableGrammartype (which would ideally be crammed back toGrammarFunction)blacken