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

Rename comment{Start,End} to multiLineComment{Start,End}, disallow newline characters in comment{Start,End} #168

Open
stefanradziuk opened this issue Feb 10, 2023 · 1 comment · May be fixed by #223
Labels
enhancement New feature or request major This change would affect break backwards compatibility
Milestone

Comments

@stefanradziuk
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The parsley.token.descriptions.SpaceDesc params commentStart and commentEnd should more clearly indicate that they are intended for multiline comments. With current naming you might try to define Wacc/Python-style comments as:

commentStart = "#",
commentEnd = "\n",

However, this should instead be defined using commentLine = "#".

Additionally, including \n in commentEnd in this case essentially causes comment lines to be discarded when calculating token positions.

Describe the solution you'd like

  • comment{Start,End} should be named more descriptively, for example multiLineComment{Start,End}.
  • Either position calculation should be fixed for comment{Start,End} containing \n, or this behaviour should be documented and produce a warning / error.
@stefanradziuk stefanradziuk added the enhancement New feature or request label Feb 10, 2023
@j-mie6 j-mie6 added this to the Parsley 5 milestone Feb 11, 2023
@j-mie6 j-mie6 self-assigned this Feb 11, 2023
@j-mie6
Copy link
Owner

j-mie6 commented Feb 11, 2023

This is a breaking change to the description, so I'll put it on the list for parsley 5.

Ruling out \n (and \t) in any lexical descriptions that assume that the characters are "flat" is something I can add as requirements on the descriptions for 5.0 onwards, and I might consider some kind of suppressible warning system that perhaps I can add in 4.3

@j-mie6 j-mie6 added the major This change would affect break backwards compatibility label Feb 11, 2023
@j-mie6 j-mie6 linked a pull request Jan 10, 2024 that will close this issue
40 tasks
@j-mie6 j-mie6 removed their assignment Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major This change would affect break backwards compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants