Skip to content
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

dump formulas as s-exprs #169

Merged
merged 1 commit into from
Oct 28, 2024
Merged

dump formulas as s-exprs #169

merged 1 commit into from
Oct 28, 2024

Conversation

digama0
Copy link
Member

@digama0 digama0 commented Oct 28, 2024

Currently, --dump-formula dumps formulas pretty-printed to token sequences, which defeats the purpose of parsing them in the first place. This makes the command more useful by printing the output as a sexpr instead, so it is easier to see the actual parse tree that resulted.

@digama0 digama0 requested a review from tirix October 28, 2024 05:53
@tirix
Copy link
Collaborator

tirix commented Oct 28, 2024

I originally used this --dump-formula as a test for the parser, as well as for the formula renderer, to compare that the dumped formulas were the same as the initial formulas. This was later automatized in grammar::verify.

In this comment it seems @GinoGiotto may have used this function.

MM formulas are easier to read for humans, so there is a use.
S-expressions are of course better suited to understand the inner workings of the grammar parser and are easier to be parsed by a computer (!).

Maybe both modes could cohabit, with a different option for each?

@digama0
Copy link
Member Author

digama0 commented Oct 28, 2024

Ping @GinoGiotto. I think given that verify-parse-stmt exists there is very little other use for something that you could accomplish with a sed script.

@GinoGiotto
Copy link

GinoGiotto commented Oct 28, 2024

I don't need --dump-formula anymore. I was interested in getting a complete list of statements from a database. That is now accomplished by --list-statements #150 so the change in this PR is fine for me.

@tirix
Copy link
Collaborator

tirix commented Oct 28, 2024

Ok, then no problem.
Change is minimal and looks good to me.
It does of course expose the inner encoding of the tree, which you are changing in #173

@tirix tirix merged commit 5908b0b into main Oct 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants