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

Try dotnet-project-file-analyzers #143

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Try dotnet-project-file-analyzers #143

wants to merge 4 commits into from

Conversation

sungam3r
Copy link
Member

No description provided.

@Corniel
Copy link

Corniel commented Dec 23, 2024

Nice that you want to use the .NET project file analyzers!

Two tips:

  1. Those rules should be applied on on MS build project files, not only those who are publisable.
  2. Consider using our SDK as a replacement for Solution Items in your solution file.

@@ -24,15 +24,24 @@
<ImplicitUsings>enable</ImplicitUsings>
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>../../assets/Destructurama.snk</AssemblyOriginatorKeyFile>
<NoWarn>$(NoWarn);Proj0003;Proj0010;Proj0028;Proj0200;Proj0201;Proj0205;Proj0206;Proj0208;Proj0213;Proj0214;Proj0215;Proj0216;Proj0240;Proj0243;Proj0400;Proj0800;Proj1001;Proj1003;</NoWarn>
Copy link

Choose a reason for hiding this comment

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

If you want to disable these, you better use a .globalconfig file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to use editorconfig for all projects I participate of. For now it's just a first attempt to use analyzer. I evaluate pros and cons. My goal is to find reasonable set of diagnostics and those ones that have a little value.

Copy link

Choose a reason for hiding this comment

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

I prefer to use editorconfig for all projects I participate of. For now it's just a first attempt to use analyzer. I evaluate pros and cons. My goal is to find reasonable set of diagnostics and those ones that have a little value.

That is true. However, MS itself states that the configuration of files should be specified in the .globalconfig instead of the .editorconfig. Some rules (including the of .NET project file analyzers) can not be configured in .editorconfig, but can be configured in .globalconfig.

Copy link
Member Author

Choose a reason for hiding this comment

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

MS itself states that the configuration of files should be specified in the .globalconfig instead of the .editorconfig

Could you please point to the relevant documentation?

Copy link

Choose a reason for hiding this comment

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

@sungam3r
Copy link
Member Author

@Corniel regarding

Those rules should be applied on on MS build project files, not only those who are publisable.

What do you mean? I added analyzer into Directory.Build.props applying it to all projects.

Consider using our SDK as a replacement for Solution Items in your solution file.

I've read about it. For now it looks like rather invasive solution. I would not want to lose the usual structure of many projects without good reason.

@Corniel
Copy link

Corniel commented Dec 23, 2024

What do you mean? I added analyzer into Directory.Build.props applying it to all projects.

<ItemGroup Condition="'$(IsPackable)' == 'true'">
    <PackageReference Include="DotNetProjectFile.Analyzers" Version="1.5.2" PrivateAssets="all" />
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" />
    <PackageReference Include="MinVer" Version="6.0.0" PrivateAssets="All" />
  </ItemGroup>

So, the DotNetProjectFile.Analyzers are now only applied on projects that are packable.

I've read about it. For now it looks like rather invasive solution. I would not want to lose the usual structure of many projects without good reason.

I have to admit that is the response I got quite often when suggesting it. And when I came up with the idea, it really felt like it. But in practice, you get a way less cumbersome solution file, and an easy to maintain container project just for files shared by the solution. That being said, if you want to give it a try, you probably should do it in a separate PR.

src/Directory.Build.props Outdated Show resolved Hide resolved
@sungam3r
Copy link
Member Author

So, the DotNetProjectFile.Analyzers are now only applied on projects that are packable.

ooops, my oversight

@sungam3r
Copy link
Member Author

But in practice, you get a way less cumbersome solution file, and an easy to maintain container project just for files shared by the solution.

I doubt it very much, especially in the words less cumbersome solution file. I do not see difficulty in the current project format. What cumbersome solution file are you talking about?

That being said, if you want to give it a try, you probably should do it in a separate PR.

Could you please provide PR to demonstrate so I can evaluate?

@sungam3r
Copy link
Member Author

I have to admit that is the response I got quite often when suggesting it.

Yep, I expected such an answer.

sungam3r and others added 3 commits December 23, 2024 19:50
Co-authored-by: Corniel Nobel <corniel@gmail.com>
# Conflicts:
#	src/Directory.Build.props
Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b5864b2) to head (1fb63db).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #143   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines          242       242           
  Branches        37        37           
=========================================
  Hits           242       242           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sungam3r
Copy link
Member Author

That being said, if you want to give it a try, you probably should do it in a separate PR.

Honestly I do not understand what I will get.

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