-
-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for when providing a transformer with a Token #1395
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1395 +/- ##
==========================================
+ Coverage 89.39% 89.41% +0.01%
==========================================
Files 52 52
Lines 7730 7744 +14
==========================================
+ Hits 6910 6924 +14
Misses 820 820
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -158,7 +158,11 @@ def _transform_tree(self, tree): | |||
|
|||
def transform(self, tree: Tree[_Leaf_T]) -> _Return_T: | |||
"Transform the given tree, and return the final result" | |||
return self._transform_tree(tree) | |||
res = list(self._transform_children([tree])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has the side effect of also allowing everything that isn't a tree or token to be passed without error. Not sure if that is good behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's bad about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might hide user error where they pass things that aren't the result of a Lark.parse
call, which is probably never intentional. But this isn't a strong objection, if you think this is fine, go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, but I think I'll keep it. My reasoning is that:
-
We allow Lark.parse to return an arbitrary object (through another transformer)
-
It would make the root behave the same as the leaf, in which we allow arbitrary objects and just skip them without an error
-
It's not the kind of mistake that is likely to proceed much without being noticed down the line. (though I admit in some edge cases it might happen)
Fixes issue #1394