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

WHITESPACE and COMMENT require $ to be compound #966

Closed
NyalephTheCat opened this issue Jan 12, 2024 · 4 comments
Closed

WHITESPACE and COMMENT require $ to be compound #966

NyalephTheCat opened this issue Jan 12, 2024 · 4 comments
Labels

Comments

@NyalephTheCat
Copy link

Describe the bug
When writing a whitespace or a comment rule, you require an explicit $ to get the inner rules.

To Reproduce
Steps to reproduce the behavior:

  • Create a rule comment like:
COMMENT = { SingleLineComment }
SingleLineComment = { "//" ~ (!"\n" ~ ANY) }

Expected behavior
You expect to get a COMMENT containing a SingleLineComment rule

Additional context
This is useful when you want to do some whitespace and comment preserving grammars. There is a way to do it the expected way: COMMENT = ${SingleLineComment} but it isn't written anywhere in the book that COMMENT and WHITESPACE behave differently than any other rules by default

@tomtau
Copy link
Contributor

tomtau commented Jan 29, 2024

Should this be fixed or just documented in the book? I worry that there may be some code depending on this special behaviour

@HoloTheDrunk
Copy link
Contributor

HoloTheDrunk commented Jan 29, 2024

As this seems pretty unintuitive, it's probably best to fix it in a major release (whenever that happens) and leave a note about this in the book.
Code that depends on it shouldn't be affected – unless of course the users are not respecting semver recommendations, at which point it's on them anyways.

@tomtau
Copy link
Contributor

tomtau commented Jan 30, 2024

it's probably best to fix it in a major release (whenever that happens) and leave a note about this in the book.

the next major release will likely have a major API and grammar overhaul, so I'm not sure. I opened a draft PR with a fix under a feature flag, but I feel it may not be worth it, especially that it's not the default (i.e. the person needs to be aware of that feature flag in the first place)... some options:

  1. keep it as it is and just document in the book
  2. put it under a dedicated feature flag (as in that draft PR)
  3. introduce dedicated grammar rules for a different behaviour (I guess this isn't much improvement over just doing ${...})
  4. put it under the "grammar-extras" feature flag

(plus document it in the book)

Maybe the last option seems fair, as "grammar-extras" already contains a bug fix for that weird trailing whitespace issue, so people using that flag are aware of these semantic breaking changes and may appreciate a more intuitive default here.
Or just leave it as it is until the next major release?

@tomtau
Copy link
Contributor

tomtau commented Feb 7, 2024

added a note to the book: pest-parser/book@f4ad611
feel free to re-open the issue if you think it's worth changing the functionality under a feature flag

@tomtau tomtau closed this as completed Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants