search

DotNetAnalyzers / StyleCopAnalyzers

An implementation of StyleCop rules using the .NET Compiler Platform
MIT License
2.64k stars 507 forks source link

Review and update code fixers to use correct new line character sequence #3656

Open bjornhellander opened 1 year ago

bjornhellander commented 1 year ago

There is a bunch of code fixers which either always use CRLF when adding line breaks, or get the character sequence from document.Project.Solution.Workspace.Options.GetOption(FormattingOptions.NewLine, LanguageNames.CSharp) and thereby fail to honor a end_of_line setting in .editorconfig.

I see these:

I assume the primary updates should be to either look at surrounding text and use the existing new line sequence or read from an .editorconfig file. What is the wanted behaviour for the ones listed above?

bjornhellander commented 1 year ago

I would vote for ignoring any config files and auto-detecting the wanted new line sequence for all analyzers, like the proposed solution for #3360.

manfred-brands commented 1 year ago

According to a comment Roslyn formats codefixes. This came up in the nunit.analyzers and as far as I tested it that seems to be the case.

MartyIX commented 1 year ago

I would vote for ignoring any config files and auto-detecting the wanted new line sequence for all analyzers, like the proposed solution for #3360.

What is the advantage of that approach? I mean if a user specifies in .editorconfig a newline setting, it seems like that's what he/she really wants to achieve.

bjornhellander commented 1 year ago

@MartyIX It could be specified there, or in a .globalconfig, or as an IDE setting. Simply adapting to what is already used would make it work without having to worry about different ways of configuring newlines. As long as the file is consistent, that is. But if it's not, then would this shortcut really make it worse? :-) Just an idea.

bjornhellander commented 1 year ago

Very interesting @manfred-brands. I have seen "elastic" being mentioned, but I have apparently not understood. Will check it out!

bjornhellander commented 1 year ago

Also see this comment regarding the probably incomplete method for detecting the actually used newline sequence, if more code fixers are updated to use it: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/pull/3607/files#r1237263110

sharwell commented 1 year ago

I would vote for ignoring any config files and auto-detecting the wanted new line sequence for all analyzers, like the proposed solution for #3360.

What is the advantage of that approach? I mean if a user specifies in .editorconfig a newline setting, it seems like that's what he/she really wants to achieve.

It's not the responsibility of every analyzer to enforce line endings for the full file. Each code fix should only fix the issue at hand; even if a file deviates from the end_of_line setting in .editorconfig, the code fix should avoid causing a file to go from consistent line endings to mixed line endings.

sharwell commented 1 year ago

Very interesting @manfred-brands. I have seen "elastic" being mentioned, but I have apparently not understood. Will check it out!

Elastic trivia is formatted by the IDE and the end of code fix applications. StyleCop Analyzers sometimes avoids using elastic trivia (e.g. via the WithoutFormatting extension) because its fixes need absolute control over the whitespace in the result.