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

Fixing Issue 186, scanner should account for newline characters when … #210

Merged
merged 5 commits into from
Mar 31, 2021

Conversation

rspruyt-google
Copy link
Contributor

…processing multi-line text. Without this source annotations line/column number (for this and all subsequent tokens) is inconsistent with plain text editors. e.g. #186. This addresses the issue specifically for single and double quote text only.

1.) We would have preferred to avoid introducing an additional state in the loop (isNewLine), but this requires placing the column marker to an invalid position (0) and believe it is safer not to do so. This is done to avoid potential bugs in the future if the loop exits before the column marker can be brought back in-bounds.

2.) We debated internally whether the slice sorting in the tests should be done through a helper function or not. We didn't feel a convenience function improved code readability to merit consolidation of the logic. If you disagree, lmk. We add the sort to be token order agnostic in our tests.

…processing multi-line text. Without this source annotations line/column number (for this and all subsequent tokens) is inconsistent with plain text editors. e.g. goccy#186. This addresses the issue specifically for single and double quote text only.
@rspruyt-google rspruyt-google marked this pull request as ready for review March 10, 2021 15:30
@goccy
Copy link
Owner

goccy commented Mar 13, 2021

Thank you for the contribution !
I'm wondering if it will affect the behavior when processing YAML containing multiline with the printer package,
so could you add a test to the printer package as well ?

@rspruyt-google
Copy link
Contributor Author

sync and had to add missing address missing name/email for CI checkout

@goccy
Copy link
Owner

goccy commented Mar 31, 2021

@rspruyt-google Great !! Thank you !!

@goccy goccy merged commit 37544cc into goccy:master Mar 31, 2021
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.

2 participants