search

DotNetAnalyzers / StyleCopAnalyzers

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

Add a setting to enable stricter handling of multiline parameters in SA1116 and SA1117 #3869

Closed mathew-tolj closed 4 months ago

mathew-tolj commented 5 months ago

Fixes #3183 by adding a new configuration setting that can be enabled to use stricter handling of multiline parameters in SA1116 and SA1117.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.39%. Comparing base (ff5c432) to head (75156ee).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3869 +/- ## ========================================== - Coverage 97.45% 94.39% -3.06% ========================================== Files 926 926 Lines 110237 110351 +114 Branches 3315 3312 -3 ========================================== - Hits 107433 104169 -3264 - Misses 1837 5171 +3334 - Partials 967 1011 +44 ```
bjornhellander commented 5 months ago

A note to @sharwell if this PR seems good: These two changed analyzers register syntax actions that fire a lot. Since they require a settings object with this change and we don't fully cache settings yet on master, I think it would be a good idea to handle #3648 first, so performance isn't negatively affected. Just a suggestion.

sharwell commented 4 months ago

I'm leaning towards not making this change. Options produce a combinatorial explosion in the test space, contributing to maintainability problems down the road (it's been a major source of problems for the Roslyn formatter). I would recommend the project looking for this change instead make a small but separate analyzer to handle this case.

mathew-tolj commented 4 months ago

I did originally have a different approach where I implemented the new behavior as disabled alternate rules (SX1116 and SX1117). That approach removes the issue with using options at the expense of a bit more duplicated code. Should I submit a pull request for that approach instead or is that unlikely to gain any traction either?

sharwell commented 4 months ago

@mathew-tolj Note that I was not referring to the creation of new rules in this project, but rather an independent project in a different repository. Projects can reference more than one analyzer package simultaneously.