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

Replaced earley by pacomb in the "metavar" example #26

Merged
merged 1 commit into from
May 1, 2024

Conversation

craff
Copy link
Collaborator

@craff craff commented Feb 18, 2023

Thanks to this PR bindlib and its examples compile under ocaml 5.0

rlepigre

This comment was marked as outdated.

@craff
Copy link
Collaborator Author

craff commented Feb 19, 2023

That looks good. I'm wondering: do we need to change the comment syntax in the example?

Just the pacomb does not have (yet) a predefined blank notion with opening/closing comments. It is always tricky
because of strings, so for an example I felt one line comments are simpler.

Also, could we keep the two parsers, and compile whichever depending on whether earley or pacomb is available? We could also add a menhir parser.

If you know how to write a conditional opam file that does not force the user to install 3 parsing technologies. I still like more pacomb or earley because the parser is both readable and inside the file. By the way I wanted to make a compile time version of earley that would not allow dynamical grammar but that could be faster than camlyacc and menhir.

@craff
Copy link
Collaborator Author

craff commented Feb 19, 2023

Remark: we needed the lateest pacomb which is not on opam. I fixed that, but I think we need the metavar test to run. I am adding this.

@rlepigre
Copy link
Owner

You'll need to rebase your MR on master, with something like git fetch && git rebase origin/master, and then force-push to the branch.

@craff
Copy link
Collaborator Author

craff commented Feb 19, 2023

bindlib and metatar tests work on 4.07, but with deps Timed >= 1.0, opam is not able to select Timed 1.0 on ocaml 4.07 and 1.1 on 4.08 and more recent. If you know how to fix that (or fix Timed 1.1), we can put back 4.07 support.

@craff
Copy link
Collaborator Author

craff commented Feb 20, 2023

All test run. I stop for now, you may review when you have time and try to recover 4.07, it is not nice to loose a version just because of Timed used in one example. Cheers.

@rlepigre rlepigre force-pushed the example_with_pacomb branch from 21c79c3 to 787a7e5 Compare February 20, 2023 07:47
@craff
Copy link
Collaborator Author

craff commented Apr 4, 2023

Hello Rodolphe ... I just did a PR on top of that one for issue #29. Do you think we should merge that one, or there is still some adjustment ?

@rlepigre
Copy link
Owner

rlepigre commented Apr 4, 2023

I don't remember where this is at, I thought I was waiting for changes on your side. I'll have a closer look when I have a moment.

@craff
Copy link
Collaborator Author

craff commented Apr 4, 2023

I think the only thing to do was recover 4.07, and it seems to work now. I just needed to ask for dune 2.0, not 2.7, but it seems to work.

@rlepigre rlepigre force-pushed the example_with_pacomb branch from 1de32b2 to f051a0a Compare April 4, 2023 22:37
@rlepigre rlepigre changed the title Replaced earley by opam in examples/metavar Replaced earley by pacomb in the "metavar" example Apr 4, 2023
@craff
Copy link
Collaborator Author

craff commented Apr 30, 2024

On en est où avec cette PR...

@rlepigre
Copy link
Owner

On en est où avec cette PR...

Aucune idée, j'ai perdu tout contexte. Je regarderai à nouveau quand j'ai un moment.

@rlepigre rlepigre force-pushed the example_with_pacomb branch from f051a0a to 3f816be Compare May 1, 2024 06:23
Copy link
Owner

@rlepigre rlepigre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@rlepigre rlepigre merged commit 0a3fed2 into master May 1, 2024
20 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.

2 participants