add concrete visitor methods #45
Conversation
danieleades
force-pushed
the
fix/add-visitors
branch
4 times, most recently
from
ff25db2
to
b09eb13
Compare
danieleades
force-pushed
the
fix/add-visitors
branch
2 times, most recently
from
5073088
to
232129f
Compare
danieleades
force-pushed
the
fix/add-visitors
branch
from
232129f
to
f5c420d
Compare
|
@tk0miya this is ready for review |
|
ping @tk0miya |
| class SparseNodeVisitor(NodeVisitor): | ||
| def visit_Text(self, node: Text) -> None: ... | ||
| def depart_Text(self, node: Text) -> None: ... | ||
| def visit_abbreviation(self, node: abbreviation) -> None: ... |
There was a problem hiding this comment.
I feel using a concrete node class for each visit/departure method is too strict. Surely it's very accurate and correct. But it warns if we'll pass a node to another visit/departure method like following:
def visit_mynode(self, node: mynode) -> None:
self.visit_emphasis(node) # => incompatible type error!
This is the reason why we use `nodes.Element` for the annotation of writers.
There was a problem hiding this comment.
i'm not sure i understand what you mean? the example you give is inherently not type-safe. The caller can't assume that visit_emphasis will even work correctly on their custom Element. In the general case, visit_X might rely on attributes/behaviour that are defined in X and not defined on the Element baseclass. This should be an error.
If a user defines a custom Element for which they wish to use visit_emphasis then surely they should subclass nodes.Emphasis (in which case this won't give an error)? Failing that, they should explicitly suppress type errors for that function/line
There was a problem hiding this comment.
These are examples.
https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/_html_base.py#l1310
https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/latex2e/__init__.py#l1622
https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/manpage.py#l475
In the general case, visit_X might rely on attributes/behaviour that are defined in X and not defined on the Element baseclass.
Generally, it's correct. But, in the docutils case, there are no nodes having additional attributes and behaviors. There is no advantage to use a detailed class for visitor methods.
There was a problem hiding this comment.
thankyou for the examples @tk0miya! I'll try to take a proper look in the next couple of days
There was a problem hiding this comment.
https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/manpage.py#l475
perhaps in this case, this would be better typed as Admonition, than admonition?
There was a problem hiding this comment.
perhaps in this case, this would be better typed as Admonition, than admonition?
The classes starting with capital are not "node" classes. They're mainly used for classifying the semantics of nodes. They don't have any methods and attributes. So typing as Admonition is not useful at all.
Additionally, the caution node is not a child of the admonition node.
add concrete visitor methods to the
GenericNodeVisitorandSparseNodeVisitorclasses