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

add duration parsing #25

Draft
wants to merge 2 commits into
base: wip-merge-pr-5
Choose a base branch
from
Draft

Conversation

Swrup
Copy link

@Swrup Swrup commented Feb 8, 2024

Hello,

This PR based on this draft adds duration parsing

@Swrup Swrup requested a review from c-cube as a code owner February 8, 2024 09:53
@Swrup Swrup marked this pull request as draft February 8, 2024 09:53
@zapashcanon
Copy link
Collaborator

Hi,

Could you describe what is left to do ?

After a first quick look: could you use a more modern Menhir syntax ? Using $1 instead of named values is hard to read IMO.

@sagotch
Copy link
Collaborator

sagotch commented Mar 8, 2024

Hi,

Could you describe what is left to do ?

After a first quick look: could you use a more modern Menhir syntax ? Using $1 instead of named values is hard to read IMO.

Regarding how trivial the parser is, shouldn't it directly de implemented in the lexer, which would avoid the menhir dependency? Introducing this new dependency is not really a problem, but I find it to be overkill for what it does.

@Swrup
Copy link
Author

Swrup commented Mar 8, 2024

that would be nice, but I don't know how to do it in just ocamllex

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