Skip to content

Conversation

singhashish-wpf
Copy link
Contributor

@singhashish-wpf singhashish-wpf commented Jun 12, 2024

Enable .NET analyzers and Upgrade langversion

This is an experimental effort, NOT to be merged.

Microsoft Reviewers: Open in CodeFlow

@singhashish-wpf singhashish-wpf added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 12, 2024
@singhashish-wpf singhashish-wpf requested a review from a team as a code owner June 12, 2024 09:55
@dotnet-policy-service dotnet-policy-service bot added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 12, 2024
@JeremyKuhne
Copy link
Member

Consider using https://github.com/dotnet/winforms/blob/main/.editorconfig as a starting point for what you enable/disable. You can drop multiple ones in your tree as you work through things, just remove root = true lower in the tree. Feel free to ping me on rationale for any rules.

@ThomasGoulet73
Copy link
Contributor

Should this PR use the existing infrastructure for code analysis by setting EnableAnalyzers to true instead of doing it manually in the project ? This would use the same analyzers as System.Xaml (The only project using it right now AFAIK). There is also an already configured config file for the rules (Which I copied from WinForms).

@sharwell sharwell marked this pull request as draft June 13, 2024 13:59
[*.{cs,vb}]
charset = utf-8-bom
insert_final_newline = true
trim_trailing_whitespace = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting can change the semantic meaning of code. It should never be set to true.

Suggested change
trim_trailing_whitespace = true
trim_trailing_whitespace = false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing spaces in a verbatim string would be one example. The C# formatter removes non-semantic trailing whitespaces automatically. The only difference that should exist between trim_trailing_whitespace and the formatter is trim_trailing_whitespace also removes trailing whitespaces that change the meaning of code. Other differences would be considered formatter bugs that Roslyn intends to fix.

dotnet_style_namespace_match_folder = true:suggestion
dotnet_style_operator_placement_when_wrapping = beginning_of_line
tab_width = 4
end_of_line = crlf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end_of_line should not be set in .editorconfig

Suggested change
end_of_line = crlf


[*.vb]
# Modifier preferences
visual_basic_preferred_modifier_order = Partial,Default,Private,Protected,Public,Friend,NotOverridable,Overridable,MustOverride,Overloads,Overrides,MustInherit,NotInheritable,Static,Shared,Shadows,ReadOnly,WriteOnly,Dim,Const,WithEvents,Widening,Narrowing,Custom,Async:suggestion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 For readability, probably better to just leave the default for this one (by omitting the line here) unless it's being changed.

Comment on lines +19 to +20
<PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="5.0.0" />
<PackageReference Include="Microsoft.Net.Compilers.Toolset" Version="4.8.0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are really old versions

@@ -9,12 +9,23 @@
<ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch>None</ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch>
<Platforms>AnyCPU;x64;arm64</Platforms>
<GenerateDependencyFile>false</GenerateDependencyFile>
<LangVersion>latest</LangVersion>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 latest is a moving target and not a real version. Would recommend using a numeric value for build stability across users who work on the project.

@singhashish-wpf singhashish-wpf closed this by deleting the head repository Jul 19, 2024
@singhashish-wpf
Copy link
Contributor Author

Closing this in favor of using the existing infrastructure.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
draft * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants