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

fix: Limit error messages new line visualization (fixes #362) #609

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Jul 1, 2022

new_from_pos and new_from_span only visualize newline
characters if positions point to the newline characters.

@tomtau tomtau force-pushed the fix/error-msg-nl-visualization branch from b8c7823 to 236d27d Compare July 1, 2022 07:18
`new_from_pos` and `new_from_span` only visualize newline
characters if positions point to the newline characters.
@tomtau tomtau force-pushed the fix/error-msg-nl-visualization branch from 236d27d to c35a751 Compare July 1, 2022 11:15
@glyn
Copy link

glyn commented Jul 1, 2022

Will conditional handling of newline characters violate the principle of least surprise?

Is it possible to use pest to parse a grammar in which newlines characters are not treated as whitespace? That may be an example where the principle of least surprise is most dramatically violated.

Maybe an example, before and after this fix, would help demonstrate that the behaviour is never surprising.

@CAD97
Copy link
Contributor

CAD97 commented Jul 1, 2022

This change (should) only impacts error rendering. When showing a snippet of source, the newlines are still visible as, well, newlines. The addition of is only needed for an error span to point at the newline character itself.

One potential source of confusion is if appears literally in the source. This PR (should) only make things better in this case, as it removes insertion of when it is not necessary in order to be able to point directly at the .

@glyn
Copy link

glyn commented Jul 1, 2022

Ok, sounds reasonable. Thanks. @tomtau feel free to add an example if you think it clarifies things.

@CAD97 CAD97 requested review from a team and jhpratt and removed request for a team July 1, 2022 18:13
@CAD97
Copy link
Contributor

CAD97 commented Jul 1, 2022

(I assigned @pest-parser/triage, which is set to automatically load-balance review requests. Feel free to reassign if you don't want to review. If we get a LGTM/Approve review from triage and CI is green (ignoring coverage, which is broken) I'll hit the merge button.)

@jhpratt jhpratt requested review from CAD97 and removed request for jhpratt July 1, 2022 22:54
@jhpratt
Copy link

jhpratt commented Jul 1, 2022

Yeah, I haven't had a chance to look through any of the code yet, so I wouldn't know what I'm looking at at this point.

Copy link
Contributor

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

In retrospect I'm not completely certain I agree with the choice to visualize LF in this manner, but this is clearly an improvement on the status quo.

@CAD97 CAD97 merged commit 96eace8 into pest-parser:master Jul 1, 2022
@tomtau
Copy link
Contributor Author

tomtau commented Jul 2, 2022

@glyn one of the error tests has an example where the character is visualized

"1 | abcdef␊",
because the error position points to \n, while some of the other error tests were fixed not to visualize the newline characters in their examples, because their positions didn't point to \n or \r.

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.

4 participants