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

Support informal delimiter literals #64

Merged
merged 2 commits into from Jun 7, 2020
Merged

Conversation

ammar
Copy link
Owner

@ammar ammar commented Jun 6, 2020

Interprets the {, }, and ] delimiters as literals depending on the context.

Notes

  • Reduce ambiguity in the quantifier_interval pattern by replacing the optional matchers with 4 fixed patterns.
  • Remove the premature_end_error final state handler from quantifier_interval to permit valid literal sequences.

Closes #63

@ammar ammar requested a review from jaynetics June 6, 2020 13:24
Copy link
Collaborator

@jaynetics jaynetics left a comment

Choose a reason for hiding this comment

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

looks great!

i've checked some more edge cases and found no issues. maybe we should add one example to spec/parser/quantifiers_spec.rb to document that leading zeros are correctly ignored as in MRI:

include_examples 'quantifier', /a{004}+b/, '{004}+', :possessive, :interval, 4, 4

Comment on lines 65 to 73
quantifier_exact = range_open . (digit+) . range_close . quantifier_mode?;
quantifier_minimum = range_open . (digit+) . ',' . range_close . quantifier_mode?;
quantifier_maximum = range_open . ',' . (digit+) . range_close . quantifier_mode?;
quantifier_range = range_open . (digit+) . ',' . (digit+) .
range_close . quantifier_mode?;

quantifier_interval = quantifier_exact | quantifier_minimum |
quantifier_maximum | quantifier_range;

Copy link
Collaborator

Choose a reason for hiding this comment

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

slightly more concise:

Suggested change
quantifier_exact = range_open . (digit+) . range_close . quantifier_mode?;
quantifier_minimum = range_open . (digit+) . ',' . range_close . quantifier_mode?;
quantifier_maximum = range_open . ',' . (digit+) . range_close . quantifier_mode?;
quantifier_range = range_open . (digit+) . ',' . (digit+) .
range_close . quantifier_mode?;
quantifier_interval = quantifier_exact | quantifier_minimum |
quantifier_maximum | quantifier_range;
quantity_exact = (digit+);
quantity_minimum = (digit+) . ',';
quantity_maximum = ',' . (digit+);
quantity_range = (digit+) . ',' . (digit+);
quantifier_interval = range_open . ( quantity_exact | quantity_minimum |
quantity_maximum | quantity_range ) . range_close .
quantitfier_mode?;

but this is more a matter of taste, ragel probably optimises away the redundancy (?)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I like it a lot. The intention is clearer. There's a typo in the name of the mode pattern so I will make the change manually.

@ammar
Copy link
Owner Author

ammar commented Jun 7, 2020

Thank you for the review. Very helpful.

i've checked some more edge cases and found no issues. maybe we should add one example to spec/parser/quantifiers_spec.rb to document that leading zeros are correctly ignored as in MRI:

include_examples 'quantifier', /a{004}+b/, '{004}+', :possessive, :interval, 4, 4

Good call. I will add it.

I will wrap this up and push out a new release (1.8) this afternoon.

@ammar
Copy link
Owner Author

ammar commented Jun 7, 2020

On second thought, I'll consider this a bug fix, and release as 1.7.1.

@ammar ammar merged commit ae68d66 into master Jun 7, 2020
@jaynetics jaynetics deleted the support-delimiter-literals branch January 4, 2022 21:59
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.

Rethink fallbacks for formally incorrect grammar
2 participants