Miscellaneous improvements to the behavior and documentation of Visitor, Transformer, Interpreter, and friends#1543
Conversation
| from inspect import isclass | ||
| if isclass(obj) and issubclass(obj, Visitor): | ||
| raise TypeError("v_args cannot be applied to Visitor classes.") | ||
| if callable(obj) and not isclass(obj): | ||
| @wraps(obj) | ||
| def method_wrapper(*args, **kwargs): | ||
| if args: | ||
| self_instance = args[0] | ||
| if isinstance(self_instance, Visitor): | ||
| raise TypeError("v_args cannot be applied to Visitor methods.") | ||
| return obj(*args, **kwargs) | ||
| return _apply_v_args(method_wrapper, func) |
There was a problem hiding this comment.
This is incorrect in at least one way: We want this to error out on any classes other than Transformer and Interpreter (or their subclasses), not just Visitor. But I am struggling to fix this without causing other tests to fail; I don't really follow this code well to begin with, and would be OK just stripping it out.
There was a problem hiding this comment.
@erezsh - Would you like me to remove this new logic as well, or do you have a suggestion as to how we could make it better?
There was a problem hiding this comment.
@erezsh - Checking in again here. Happy to cut this PR back to just documentation changes if you prefer that.
Can you explain this comment? Interpreter.top_down() would just use the |
|
Ah, I think I misinterpreted the behavior and jumped to the wrong conclusion. Is the behavior of |
|
@nchammas Well, I can see how it was confusing. But I don't think adding |
|
Maybe it would be better to remove the |
|
@erezsh - No rush; just wanted to check in again if I can do anything to move this PR forward. |
|
Hi @erezsh - Is there anything I can do to move this PR forward, including reducing the scope? If not, happy to just close it. |
|
Hi @nchammas , sorry, I was a bit distracted with other projects. I'll try to allocate some time for Lark soon. Do I have your permission to take the parts I like and leave out the rest? Some of the edits might remove the authorship marker from the commits, but I'll try to avoid it. |
Sure, go ahead. I have the "Allow edits by maintainers" thing enabled on this PR, so you can just push commits here if that's easiest for you. |
2 participants
This PR makes the following improvements.
Documentation:
visit_children_decorand various methods ofInterpreter.Visitorto clarify that it can traverse the tree in either direction.visit_children_decor, which was previously not visible.Behavior:
visit_children_decorraise aTypeErrorif applied to a method of any class other thanInterpreter.v_argsraise aTypeErrorif applied to aVisitoror its methods. I had to get help from GPT to figure out to make this check work. I'm not confident in the logic.