👌 IMPROVE: Make SyntaxTreeNode type annotations work when subclassing #131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
==========================================
- Coverage 96.51% 96.43% -0.08%
==========================================
Files 72 72
Lines 3183 3202 +19
==========================================
+ Hits 3072 3088 +16
- Misses 111 114 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
hukkin
changed the title
hukkin
changed the title
|
class MyTree(SyntaxTreeNodeBase["MyTree"]):
pass
tree = MyTree["MyTree"]()
EDIT: this comment is outdated |
|
If you're wondering why this has to be so complicated in the first place, haha, it's because many interfaces (method input or outputs, public class attrs) in this class reference the type of |
markdown_it/tree.py
Outdated
| closing: Token | ||
|
|
||
|
|
||
| _T = TypeVar("_T", bound="SyntaxTreeNode") |
There was a problem hiding this comment.
why _T? it would be better from a readability perspective to use something like NodeType
There was a problem hiding this comment.
It seems like its a convention to name TypeVars as T (or if private, _T). I think this is because its short, and because the same TypeVar could be reused in various contexts having a different meaning in each. Here setting the bound sort of limits any reasonable reusability though.
That being said, I don't oppose renaming at all if you think thats better 😄
markdown_it/tree.py
Outdated
| # Empty list unless a non-empty container, or unnested token that has | ||
| # children (i.e. inline or img) | ||
| self.children: List["SyntaxTreeNode"] = [] | ||
| self.children = [] |
There was a problem hiding this comment.
should this not be self._children?
There was a problem hiding this comment.
There really is no difference, but I think using the public API is less brittle in case someone ever changes the @property setter
There was a problem hiding this comment.
I think all internal variables should be set in __init__. TBH I'm surprised flake8 does not fail this, pylint definitely does
There was a problem hiding this comment.
The internal vars are set in init, but via the @Property, flake8 understands this.
There was a problem hiding this comment.
Reluctantly made this change 😄
markdown_it/tree.py
Outdated
|
|
||
| # Root node does not have self.parent | ||
| self.parent: Optional["SyntaxTreeNode"] = None | ||
| self.parent = None |
There was a problem hiding this comment.
should this not be self._parent
There was a problem hiding this comment.
same as self._children 😄
markdown_it/tree.py
Outdated
|
|
||
| @classmethod | ||
| def from_tokens(cls, tokens: Sequence[Token]) -> "SyntaxTreeNode": | ||
| def from_tokens(cls: Type[_T], tokens: Sequence[Token]) -> _T: |
There was a problem hiding this comment.
do cls and self really need to be annotated?
There was a problem hiding this comment.
Yep, the type checker binds a meaning to _T based on context. Here it gets the context from cls (it knows its type) so it deduces that the return type is the same as that.
E.g. if you call node = MyTreeNode.from_tokens(), the type checker then knows that node is MyTreeNode and if the type is something different (another subclass) the type checker binds the TypeVar to that.
There was a problem hiding this comment.
To clarify, if you dont annnotate cls, the type checker doesn't have any context useful to binding the TypeVar so the return type will be Any.
|
@chrisjsewell I added the necessary |
3 participants
This PR makesSyntaxTreeNode.childrenandSyntaxTreeNode.parentread-only properties, as it seems that's by far the least complicated way of making type annotations work expectedly when extending the class. Those properties shouldn't be rewritten anyways so I guess this is not a bad thingEdit: This PR simply makes
SyntaxTreeNode.childrenandSyntaxTreeNode.parent@propertys so that they can be type annotated in a way where the getter return typeTypeVaris automatically bound to type ofself.