-
-
Notifications
You must be signed in to change notification settings - Fork 33
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 tokenization of consecutive \r\n
line endings
#460
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
==========================================
- Coverage 96.43% 95.84% -0.60%
==========================================
Files 14 13 -1
Lines 4208 3945 -263
==========================================
- Hits 4058 3781 -277
- Misses 150 164 +14 ☔ View full report in Codecov by Sentry. |
Without this patch `\r\n\r\n` would be tokenized as `\r\n\r` and `\n` instead of `\r\n` and `\r\n`. Fixes fredrikekre/Runic.jl#15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
(Side note: One thing I've wondered is whether we should lex newline whitespace as a strictly separate token from any other whitespace. That's a separate PR though.)
Co-authored-by: Claire Foster <aka.c42f@gmail.com>
This was a bit of a surprise to me actually. Since there are both Whitespace and Newline(Ws) tokens it seems a bit strange to have them combined. On the other hand, for the purpose of writing a code formatter it was rather convenient: Stripping trailing space is easy since I only need to find NewlineWs nodes, and indents is also easy for the same reason (no need to peek backwards and forward). Also, can we have a new release including this and #455 ? Is there a reason releases aren't cut from the main branch? I am happy to do the backporting work to the release-0.4 branch but would be nice to get back to releasing from main I think. |
We can have a new release, for sure. I've been working toward that in the past couple of days - merging a few fixes and getting in some changes I need for JuliaLowering. If we make it from But I've been feeling like we should get back to releasing off |
Yes it seems kinda odd right? We don't need to change it if it's convenient. But also it doesn't seem very clean 🤷♀️ |
Which PRs/commits are breaking? |
Without this patch `\r\n\r\n` would be tokenized as `\r\n\r` and `\n` instead of `\r\n` and `\r\n`. Fixes fredrikekre/Runic.jl#15. (cherry picked from commit a11e664, PR #460)
Without this patch
\r\n\r\n
would be tokenized as\r\n\r
and\n
instead of\r\n
and\r\n
. Fixesfredrikekre/Runic.jl#15.