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

Add \r to the list of non-unescaped characters to fix windows CI #14

Closed
wants to merge 5 commits into from

Conversation

LilithHafner
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #14 (421f100) into lh/ci-old-julia (63ff132) will not change coverage.
Report is 1 commits behind head on lh/ci-old-julia.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           lh/ci-old-julia      #14   +/-   ##
================================================
  Coverage            67.21%   67.21%           
================================================
  Files                    7        7           
  Lines                  796      796           
================================================
  Hits                   535      535           
  Misses                 261      261           
Files Coverage Δ
src/stylemacro.jl 71.42% <100.00%> (ø)

@LilithHafner
Copy link
Member Author

Whelp, that fixed it. Manually listing many escape characters is typically codesmell. It is notoriously difficult to get escaping and unescaping right. @tecosaur, how do we know we've got the right set of characters to not unescape?

@LilithHafner LilithHafner marked this pull request as ready for review October 26, 2023 13:23
@LilithHafner LilithHafner changed the base branch from lh/ci-old-julia to main October 26, 2023 13:28
@tecosaur
Copy link
Collaborator

Well, each one of those was added for a particular reason. Otherwise things like {red:\}} wouldn't work.

@LilithHafner LilithHafner changed the title Add some debug statements to see what's going on with windows CI Add \r to the list of non-unescaped characters to fix windows CI Oct 26, 2023
@tecosaur
Copy link
Collaborator

What I wonder with \r is whether we need to change the special handling with \n that allows for:

styled"some content \
       split across \
       lines"

(this is currently missing a test)

@LilithHafner
Copy link
Member Author

Well, each one of those was added for a particular reason.

IIUC ['{', '}', ':', '$'] is a complete list of the special characters used in the styled macro and therefore must receive special treatment here. That makes sense.

The presence of \n and \r is weird, though.

@tecosaur
Copy link
Collaborator

\n was added because I wasn't able to make the \-continued lines work without it.

@LilithHafner
Copy link
Member Author

I was going to say "shouldn't you just use the built-in support for strings without newlines split across multiple lines?" but it seems that Julia does not have support for this 😢

@tecosaur
Copy link
Collaborator

Everything from Julia strings that "just works" here has been manually implemented 😅.

@LilithHafner
Copy link
Member Author

That's sad and error-prone. Julia should provide a way to reuse the existing string parsing machinery.

julia> "abc\
       def"
"abcdef"

julia> macro noop_str(str)
           println(str)
           str
       end
@noop_str (macro with 1 method)

julia> noop"abc\
       def"
abc\
def
"abc\\\ndef"

@LilithHafner
Copy link
Member Author

LilithHafner commented Oct 26, 2023

It looks like Base strings identify and delete things that match the regex\\(\r|\n)\s* (i.e. backslack, followed by a newline or carriage return, and then any subsequent whitespace)

https://github.com/JuliaLang/JuliaSyntax.jl/blob/1b048aad2a3a3e2e9d7f5053ff51024f08ca308c/src/parser.jl#L3354-L3358

@tecosaur
Copy link
Collaborator

I thin we're just accomplishing the same effect via state machine instead of regexp.

@LilithHafner
Copy link
Member Author

Oh, JuliaSyntax also doesn't use a Regex, that was just to state what's going on.

@LilithHafner
Copy link
Member Author

Anyway, moving discussion to #16

@tecosaur
Copy link
Collaborator

I think this is superseded by #16 being merged.

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