JuliaLang / JuliaSyntax.jl

The Julia compiler frontend
Other
274 stars 33 forks source link

Fix tokenization of consecutive `\r\n` line endings #460

Closed fredrikekre closed 4 months ago

fredrikekre commented 4 months ago

Without this patch \r\n\r\n would be tokenized as \r\n\r and \n instead of \r\n and \r\n. Fixes https://github.com/fredrikekre/Runic.jl/issues/15.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.84%. Comparing base (ae7d6ac) to head (df7221e). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fredrikekre commented 4 months ago

Side note: [...]

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.

c42f commented 4 months ago

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 main it'll be a breaking release, so I guess we may as well make it JuliaSyntax 1.0. If I don't get it done soon enough for your liking (I've been feeling overloaded!) you're welcome to backport the bugfixes to the release-0.4 branch and we can release that.

But I've been feeling like we should get back to releasing off main - having the divergence between the release and main has become a pain point.

c42f commented 4 months ago

Since there are both Whitespace and Newline(Ws) tokens it seems a bit strange to have them combined.

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 :woman_shrugging:

fredrikekre commented 4 months ago

Which PRs/commits are breaking?

c42f commented 4 months ago

325 and #322 at least. (Yiikes. Those are so old. Speaks to how much we need to get back to releasing from main)