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

csharpier-ignore comments force CRLF line endings #884

Closed
pingzing opened this issue Apr 28, 2023 · 5 comments
Closed

csharpier-ignore comments force CRLF line endings #884

pingzing opened this issue Apr 28, 2023 · 5 comments
Labels
Milestone

Comments

@pingzing
Copy link

I'm running into an interesting issue with line endings! I have a project configured to use LF line-endings, both at the .editorconfig level, and in CSharpier's csharpierrc config file. That works fine.

However! I have a block of code that I have // csharpier-ignored. When CSharpier hits that line, it replaces the LFs with CRLFs.

I'm running on Windows, and this is CSharpier v0.24.1.

Example:

No //csharpier-ignore:
image

After adding // csharpier-ignore and running dotnet csharpier .:
image

What makes things even more fun is that once this hits CI, it fails the --check check that we do, because I have git's core.autocrlf set to true, which forces all CRLFs to LFs upon pushing it up to the remote. When CSharpier's --check command runs in CI, it detects that it would change these LFs to CRLFs, and fails the build.

I'm not sure what my expected fix is here. Maybe make the newline-inserting that CSharpier (seems to?) do here respect endOfLine?

@belav
Copy link
Owner

belav commented Apr 28, 2023

This is a very odd issue but I have a hunch what may be going on. It could be that when csharpier asks Roslyn for the source of an ignored node, Roslyn is using CRLF.

CSharpier does have code to replace mismatched line endings in ignored code with what is configured, but I believe that only happens with statements in ranged ignores.

I'm out of town right now so can't verify the theory, but I can look at a fix when I'm back next week.

@belav
Copy link
Owner

belav commented May 4, 2023

I haven't been able to reproduce this. I tried on the file system as well as in a test, with a bunch of combinations of line endings in the file + line ending options.
https://github.com/belav/csharpier/blob/74924377e93fbc3ba76d4e9e4e58e98211c22c64/Src/CSharpier.Tests/FormattingTests/LineEndingEdgeCase.cs

CSharpier doesn't expose the line endings as an option in the .cshapierrc, it did originally but currently it defaults to auto, which formats each file based on the first line ending that it encounters in a given file. But that doesn't explain why adding // csharpier-ignore would cause mismatched line endings.

Could you supply the full file? Or a trimmed down version that reproduces the problem.

@pingzing
Copy link
Author

pingzing commented May 4, 2023

Sure! Here's a repository that exhibits the issue: https://github.com/pingzing/CSharpierRepro

In order to reproduce it, do the following:

  • Clone the repo
  • Change the line endings in Program.cs to LF, by running dotnet format, or using the VS line ending fixer, or really, however
  • Run dotnet csharpier .

But here's the really interesting part: If I replace the contents of .csharpierrc.json with an empty JSON object (i.e. just { }), or just delete it altogether, I can't reproduce the problem! It seems like the presence of the config file seems to change the behavior here.

EDIT: Ah--and another note: the original issue was in a repo where Git's core.autocrlf was set to false. Apologies--forgot we had a project-specific override for that one!

@belav
Copy link
Owner

belav commented May 5, 2023

It took a bit to track down, but this had to do with the combination of preprocessor symbols + csharpier-ignore not properly replacing line endings in the ignored section of code. And of course was a one line fix once I found it. And I believe it only shows up if the file in question does not use the system's default line endings (file with LF on windows which defaults to CRLF).
I'll push out a fix for this shortly!

@belav belav added this to the 0.24.2 milestone May 5, 2023
@belav belav closed this as completed in d5df8ed May 5, 2023
@pingzing
Copy link
Author

pingzing commented May 5, 2023

Fantastic work! Thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants