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

chore: Add .editorconfig file and apply settings #165

Merged
merged 4 commits into from
Feb 12, 2025

Conversation

filzrev
Copy link
Contributor

@filzrev filzrev commented Feb 6, 2025

What's changed in this PR**

  1. Add .editorconfig file to solution root.
  2. Apply dotnet format command.
  3. Apply editorconfig settings manually (Open file in VS, apply code formatter then save files)

Background

I want to suppress diffs that occurred when applying source code formatter.

Details of .editorconfig settings

1. Set source file encoding to UTF-8(without BOM)
Currently both UTF-8(with BOM) /(without BOM files are exists.
So enforce UTF-8 (without BOM) for source file encodings.

Note

I've read following post before.
If it's preferred to use utf-8 (with BOM) I'll modify settings.
https://x.com/neuecc/status/1876077902258897040

This repository's files doesn't expecting non-ASCII chars (Except for markdown files)
So I thought there is no problems using utf-8.

2.Explicitly set NewLine chars to LF
Enforce new line chars to LF.
Because C# Raw String Literals capture these chars.

3. Set indent chars to spaces and adjust indent_size
Currently csproj/props files using tab.

As far as I've confirmed on other Cysharp's repository.
It seems no strict requirements to use tab. So I've changed settings to enforce spaces.

4. Enable insert_final_newline/trim_trailing_whitespace settings

@neuecc
Copy link
Member

neuecc commented Feb 7, 2025

Thank you, most of the interpretations are fine.
Regarding the BOM discussion, yes, since Japanese isn't used in OSS projects, that approach is fine.

I might have some reservations about the end_of_line settings.
Would it be okay to remove this?
Instead, I'm thinking of standardizing line endings in the generated code by calling String.ReplaceLineEndings when using SourceProductionContext.AddSource, what do you think about this approach?

P.S.: Ah, since ReplaceLineEndings is from .NET 6, I guess it can't be used in Source Generator

@filzrev
Copy link
Contributor Author

filzrev commented Feb 7, 2025

I might have some reservations about the end_of_line settings.
Would it be okay to remove this?

Personally I want to continue using this configuration (end_of_line = lf)
Without this settings it might be committed CRLF files accidently.

And files that are generated by Source Generator already has // <auto-generated/> comment.
So these files end_of_line chars are not affected by dotnet format and VS Code formatter.

Instead, I'm thinking of standardizing line endings in the generated code by calling String.ReplaceLineEndings when using
SourceProductionContext.AddSource, what do you think about this approach?
P.S.: Ah, since ReplaceLineEndings is from .NET 6, I guess it can't be used in Source Generator

I though Source Generator files should output source files using LF regardless of OS environment.

@neuecc
Copy link
Member

neuecc commented Feb 7, 2025

What happens to the code written using StringBuilder's WriteLine?
Also, I don't think lf is that common in Windows environments
I also have git's autocrlf set to true

@filzrev
Copy link
Contributor Author

filzrev commented Feb 7, 2025

I've removed end_of_line = lf setting from .editorconfig.


Currently it seems unit test failed on following cases.

  • On Windows and checkout code with git core.autocrlf = false settings.
  • On Windows and checkout code with git core.autocrlf = true settings. Then run tests on WSL2 environment.

These issues need to be handled by #162.

@neuecc
Copy link
Member

neuecc commented Feb 12, 2025

ok, thanks!

@neuecc neuecc merged commit 077ca44 into Cysharp:master Feb 12, 2025
1 check passed
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