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

crystal tool format on Windows converts all CRLF line endings to LF #13772

Closed
kojix2 opened this issue Aug 26, 2023 · 8 comments
Closed

crystal tool format on Windows converts all CRLF line endings to LF #13772

kojix2 opened this issue Aug 26, 2023 · 8 comments

Comments

@kojix2
Copy link
Contributor

kojix2 commented Aug 26, 2023

This is usually a good thing for Linux and macOS systems, but it might not work well on Windows. This can be especially problematic if you're using Git core.autocrlf.

@kojix2 kojix2 added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Aug 26, 2023
@Blacksmoke16 Blacksmoke16 added topic:tools:formatter platform:windows Windows support based on the MSVC toolchain / Win32 API labels Aug 26, 2023
@erdian718
Copy link
Contributor

I don't think it's a bug, it's a problem with tool configuration.
Also, I see that *.cr text eol=lf has been set in .gitattributes.

@kojix2
Copy link
Contributor Author

kojix2 commented Aug 26, 2023

How can I configure crystal tool format not to automatically convert CRLF to LF ?

crysatal-lang/crystal uses .gitattributes, but I don't think it is directly related to this issue.

@erdian718
Copy link
Contributor

I mean, maybe the configuration of git and editor should be modified. Even on Windows, there's nothing wrong with using LF directly.

@kojix2
Copy link
Contributor Author

kojix2 commented Aug 26, 2023

That is correct. (I also set eol=lf in .gitattributes in some repositories. It works fine.)
Still, I don't think the behavior of crystal tool format converting CRLF to LF on Windows is quite as expected.

@straight-shoota
Copy link
Member

This behaviour is indeed intentional. See #5831 (comment) and other comments in that issue.

The purpose of the formatter is to transform Crystal source code into a standardized format that ideally everyone can live with.
It standardizes on a the single newline designator for the purpose of simplicity. LF works on all platforms. Having to deal with different line endings is an avoidable pain when working across platforms.

When working with git the recommended setting is .cr text eol=lf to make sure Crystal files are always checked out with LF.

@straight-shoota straight-shoota added status:discussion and removed kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API labels Aug 26, 2023
@kojix2
Copy link
Contributor Author

kojix2 commented Aug 31, 2023

I am by no means a CRLF enthusiast, and I agree with the policy of recommending the use of LF for practical purposes.
However, there is a certain difference between "recommended" and "no other option".

The newline codes used by the formatter are listed here, and it seems easy enough to change it to use CRLF.

result = lines.join('\n') + '\n'
result = "" if result == "\n"

--- a/src/compiler/crystal/tools/formatter.cr
+++ b/src/compiler/crystal/tools/formatter.cr
@@ -4923,8 +4923,8 @@ module Crystal
           line.rstrip
         end
       end
-      result = lines.join('\n') + '\n'
-      result = "" if result == "\n"
+      result = lines.join("\r\n") + "\r\n"
+      result = "" if result == "\r\n"
       if @shebang
         result = result[0] + result[2..-1]
       end

(I'm not sure how to compile crystals on Windows, though. )

@straight-shoota
Copy link
Member

There's always the option to use the formatter or not.

(Or your could use it and then convert newlines afterwards explicitly 🤷)

The formatter is supposed to be basic and a bit opinionated. It won't make it right for everyone. But it's intended to be acceptable by most.

Could you explain why you think it's important to use CRLF, what's your use case?

@straight-shoota
Copy link
Member

Closing due to no activity. We can reopen if there are more substantial arguments.

@straight-shoota straight-shoota closed this as not planned Won't fix, can't repro, duplicate, stale Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants