-
Notifications
You must be signed in to change notification settings - Fork 73
flattening out the parse table #111
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
Changes from all commits
ef1ba00
f48fb9d
75cab06
36a87fb
1d90bde
7095527
069b81a
92d261b
02f691d
0fd8723
3b193f0
13dad41
bd23590
4cc9382
c60bf90
0cf43fc
bcc2218
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 do we need to change to use this with
pre_visit()
instead of a "manual" recursive call?Uh oh!
There was an error while loading. Please reload this page.
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 tried that but I could not find an easy way to do that. In contrast to
pre_visit()
, we pass scalars from one level to the other, not functions. Also, unlike inpre_visit()
, we do not map over the children only, but simultaneously over other columns too (pmap()
instead ofmap()
). You could probably create a function that can accommodate both (apmap
visitor and for the usual case, we just have p = 1), but I felt it would not make things clearer since the tasks are not very similar in their implementation.Uh oh!
There was an error while loading. Please reload this page.
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.
How about:
This stores the information for the next stage of the visitor in the children, where it is then picked up as needed.
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 did this:
And changed
context_towards_terminals
toWhich could probably done more elegantly with a
map_if()
.In addition, we need to initialise the new columns in
create_filler
And in all functions that create new tokens such as
add_brackets_in_pipe
.All tests pass.
What do you think? I think initial version was more clear, did not need three new columns and had less (almost-)code duplication. Maybe there is some more simplification I missed?
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 would only add these columns temporarily in this visitor, and then remove them from the final result.
Anyway, I thought we should keep using visitors instead of recursive calls, but if the code is more difficult to understand with the visitor, let's keep the recursive calls.
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.
Another advantage of using visitors though is that we can allow the user to take control over those transformations if the transformers are passed via the
transformers
argument. Maybe we can change it later if we consider this to be a manipulation the user should control.