#7950: Lombiq.Analyzers.OrchardCore initial setup (Lombiq Technologies: OCORE-130) #15366
Conversation
|
I am not sure what is the advantage here. Probably worth mentioning some key points. Will we still need https://github.com/OrchardCMS/OrchardCore/blob/main/.editorconfig#L92-L295 ? |
See #7950.
Nope, most of the editorconfig will be removed. |
…ggestion", adding it to the solution
.editorconfig
Outdated
| @@ -1,295 +1,45 @@ | |||
| root = true | |||
| # WARNING: Only edit this file in the Lombiq .NET Analyzers repository's "Lombiq.Analyzers" folder. A copy of this file | |||
There was a problem hiding this comment.
I think we should be able to override Lombiq's project using this file. Lombiq may have perfect settings buy default, but if someone disagree, we should be able to override it here. This maybe a better way so that other people can use the Lombiq's project as their base and tweak what they don't like
There was a problem hiding this comment.
I rolled back the changes and started from scratch (in terms of editor settings).
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
There was a problem hiding this comment.
Also enable build warnings in CI builds so we don't introduce new violations. See from here, at least three switches are needed: https://github.com/Lombiq/GitHub-Actions/blob/dev/.github/actions/build-dotnet/Build-DotNetSolutionOrProject.ps1#L24
There was a problem hiding this comment.
Add the file to the solution so we can edit it from the VS GUI too.
| dotnet_diagnostic.SA1138.severity = none # Indent elements correctly. | ||
| dotnet_diagnostic.SA1139.severity = none # Use literal suffix notation instead of casting. | ||
| # AsyncFixer rules | ||
| dotnet_diagnostic.AsyncFixer01.severity = silent # Check later as warning. |
There was a problem hiding this comment.
Use suggestion instead of silent everywhere. That way, the violations will be visible but not disruptive.
| dotnet_diagnostic.CA1027.severity = warning | ||
| dotnet_diagnostic.CA1028.severity = warning | ||
| dotnet_diagnostic.CA1030.severity = warning | ||
| # Disabling "catch a more specific exception type" suggestion which is overwhelmingly a false positive. |
There was a problem hiding this comment.
Use none for disables instead everywhere.
There was a problem hiding this comment.
I've lost track here. Some of the https://github.com/Lombiq/.NET-Analyzers/blob/dev/Lombiq.Analyzers/.editorconfig, https://github.com/Lombiq/.NET-Analyzers/blob/dev/Lombiq.Analyzers/Lombiq.Analyzers.globalconfig, https://github.com/Lombiq/.NET-Analyzers/blob/dev/Lombiq.Analyzers.OrchardCore/Lombiq.Analyzers.OrchardCore.globalconfig files' content is here but some not, E.g. this and all the category-level configs would be useful too.
Furthermore, there's a lot of further setup, including config files for StyleCop and SonarLint, that would be useful here, see https://github.com/Lombiq/.NET-Analyzers/blob/dev/Lombiq.Analyzers/Build.props.
I suggest starting with these in their entirety, then working in the previously existing configs of OC, and then fine-tuning (including temporarily configuring rules for suggestion only). Those configs are the results of years of experience with the idiosyncrasies of all these analyzers (like figuring out why something shows a violation locally but not in CI) and I wouldn't want to replay those experiences.
|
Also, please checkout #15287 How many tools/projects should we need to format code. Not against this PR, but I think we need to be be careful on how many tool to adopt here before we these tools have a counter affect. Ideally a single tool with a way to override options "because we may have opinion that conflict with these tools" |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
Is this something you'd like to revisit any time soon @BenedekFarkas or should we close? |
|
I slept on it (a few times :D) - for now it seems that all of this is probably too much at once and I'm sure that across these analyzer libraries and their multitude or rules, there will be a lot of debate that isn't worth having compared to the value the rule brings. I'll focus on O1 for a while for now, so I'll close this and a new PR later can start afresh. |
No description provided.