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 CI #69

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Add CI #69

merged 4 commits into from
Nov 27, 2024

Conversation

DavisVaughan
Copy link
Collaborator

No description provided.

@DavisVaughan
Copy link
Collaborator Author

DavisVaughan commented Nov 27, 2024

---- formatter::r::specs::r::value::string_value_r stdout ----
thread 'formatter::r::specs::r::value::string_value_r' panicked at C:\Users\runneradmin\.cargo\git\checkouts\biome-d49ce8898b420a50\2648fa4\crates\biome_formatter\src\builders.rs:394:5:
The content 'r"("some raw string
business")"' contains an unsupported '\r' line terminator character but text must only use line feeds '\n' as line separator. Use '\n' instead of '\r' and '\r\n' to insert a line break in strings.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
    formatter::r::specs::r::value::string_value_r

Hmm that's this test

r"("some raw string
business")"

but that's interesting because this file was created on a mac where \n is surely the line terminator. Maybe git checks it out with \r\n instead? I think I've seen it do that before.

Regardless this feels like an interesting restriction for multiline/raw strings


On the windows vm

> brio::read_file(x)
[1] "\"hi there!\"\r\n\"\\\"\"\r\n'hi there!'\r\n'\\''\r\n\"'\"\r\nr\"(\"some raw string\r\nbusiness\")\"\r\n"

I can see that biome does have some multiline string tests
https://github.com/biomejs/biome/blob/cd1c8ec4249e8df8d221393586d664537c9fddb2/crates/biome_js_formatter/tests/specs/prettier/js/strings/multiline-literal.js

However they also set gitattributes that ensure those files are checked out with LF endings
https://github.com/biomejs/biome/blob/cd1c8ec4249e8df8d221393586d664537c9fddb2/.gitattributes

See also rome/tools#2060

@DavisVaughan
Copy link
Collaborator Author

DavisVaughan commented Nov 27, 2024

Ah interesting it looks like normalize_string() is the util for this
https://github.com/biomejs/biome/blob/cd1c8ec4249e8df8d221393586d664537c9fddb2/crates/biome_formatter/src/token/string.rs#L35

If you look at usage of that, its in LiteralStringNormaliser, used by FormatLiteralStringToken, which ultimately is used when formatting things like JsStringLiteralExpression and FormatJsxString.

So it looks like biome handles inputs with multiline strings containing \r\n by normalizing them here before hitting located_token_text(), which is where our error comes from.


It looks like biome has some kind of invariant built in where it doesn't allow \r\n within a multiline string. Only \n can be used within a multiline string.

i.e. in a case like this you are supposed to end up with this interesting mix of line endings on windows

1 + 1\r\n
r"("some raw string\n
business")"\r\n
2 + 2\r\n

But maybe this is a good thing? Right? Like, if you execute that multiline string right now, then on some platforms you get a string containing \r\n and on other platforms you get a string containing \n. It feels like the point of this is to ensure that the "contents" within the string is consistent across all platforms, and that sounds nice actually.

Same in ruff!
https://github.com/astral-sh/ruff/blob/82c01aa6623b7d4df1e4f06cb2889059f287336f/crates/ruff_python_formatter/src/string/normalize.rs#L681

Ruff PR astral-sh/ruff#5451


Right now my plan is to also implement this, but in another PR. I'll comment out the test so we can merge

This is critical for parser tests to run on Windows. Otherwise git will check them out with `\r\n` endings and you'll see a snapshot diff in the parser tests. It's what ruff and biome both do.

We will also need this file to explicitly declare `eof=crlf` on files when we want to add crlf specific tests
It had CRLF endings for some reason
@DavisVaughan DavisVaughan merged commit 61d2e71 into main Nov 27, 2024
4 checks passed
@DavisVaughan DavisVaughan deleted the feature/ci branch November 27, 2024 16:02
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.

1 participant