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

Use .NET project files analyzers #145

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

Conversation

Corniel
Copy link

@Corniel Corniel commented Dec 23, 2024

Including the use of the SDK.

Note that the solution file (and Directory.Build.props) have been move a dir up, so that all things inlcuded are not below the specfied root level).

@github-actions github-actions bot added performance Issue or pull request about performance problems tests Pull request that adds new or changes existing tests labels Dec 23, 2024
@sungam3r
Copy link
Member

Thanks, @Corniel . I will try to formulate my questions regardless of numerous changes, leading (in my opinion) to clogging projects with excessive settings.

  1. I see new (pseudo) project without any code. As you said

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.

So am I understand correctly that the whole purpose of that container/pseudo project is to be available to your analyzer/sdk?

  1. I see you added <NoWarn>$(NoWarn);NU1504</NoWarn> in .net.csproj. Without it I see Duplicate 'PackageReference' items found. Remove the duplicate items or use the Update functionality to ensure a consistent restore behavior. The duplicate 'PackageReference' items are: DotNetProjectFile.Analyzers 1.5.3, DotNetProjectFile.Analyzers 1.5.3. Why so? How to organize references correctly so that you do not have to write NoWarn?

  2. I see Define usings explicit. (https://dotnet-project-file-analyzers.github.io/rules/Proj0003.html) regardless of dotnet_diagnostic.Proj0003.severity = none configured in .globalconfig. Why?

  3. I see This <TargetFramework> will be ignored due to the earlier use of <TargetFrameworks>. (https://dotnet-project-file-analyzers.github.io/rules/Proj0027.html) because of

<PropertyGroup Condition="'$(OS)' != 'Windows_NT'">
  <TargetFramework>net9.0</TargetFramework>
</PropertyGroup>

But when I change it to TargetFrameworks then I see Use the <TargetFramework> node instead. (https://dotnet-project-file-analyzers.github.io/rules/Proj0009.html). Is this a bug?

  1. Also I see 1>\attributed\src\.vs\destructurama-attributed\config\applicationhost.config(1,1,1,2): warning Proj3000: This file is using UTF-8 encoding with BOM. (https://dotnet-project-file-analyzers.github.io/rules/Proj3000.html). As I understand such files should not be analyzed at all, right?

@Corniel
Copy link
Author

Corniel commented Dec 25, 2024

@sungam3r

  1. Yes, in a nutshell that is the reason. It allows to define files that should be analyzed for multiple projects without to have to do it twice. As bonus, it shows files (such has markdown files, .githhub files and such, without to explicitly having to add it.
  2. That is a bug/issue with the SDK project. It always includes the analyzer reference, even it that is already done by the Directory.Build.props. I have to find a way still to not to include if it already has been added.
  3. I'm not sure why that happens, it should not be the case. I had it too.
  4. FP's due to the fact that those rules do not take Condition into account.
  5. Proj3000 checks aims to check all files, not only additional files. The goal is to ignore files that are excluded by .gitignore if possible, and files under bin/ and obj/ are already excluded.

Thanks for looking into this! It is a challenge to come with suggestions/rules that fit all project setups. It helps to further improve .NET project file analyzers.

As mentioned, 4 is a FP. But what is your reasoning to solve it like this? It might give inside in how to solve these kind of things? (I considered a rule that states that TargetFramework should not be conditional in the first place)

@sungam3r
Copy link
Member

Proj3000 checks aims to check all files, not only additional files. The goal is to ignore files that are excluded by .gitignore if possible, and files under bin/ and obj/ are already excluded.

.gitignore contains .vs/, I got it, it was my old local folder after you have moved sln.

I considered a rule that states that TargetFramework should not be conditional in the first place

It is conditional rather often.

But what is your reasoning to solve it like this?

Like how? What do you mean? I just follow warning from analyzer.

@Corniel
Copy link
Author

Corniel commented Dec 25, 2024

I considered a rule that states that TargetFramework should not be conditional in the first place

It is conditional rather often.

I can't recall to see it before. But that might have to do with working on projects with different characteristics.

But what is your reasoning to solve it like this?

Like how? What do you mean? I just follow warning from analyzer.

Purely out of interest: what is the reasoning to have different targets based on the OS?

The reasoning about having conditionals only on level 1 is that it should improve readability; when defined on level 2, the value of what is conditional tends to be harder to read. Although I admit that in case of TargetFramework/TragetFrameworks, that is not per se the case.

@sungam3r
Copy link
Member

what is the reasoning to have different targets based on the OS?

It's simple - packaging and/or testing on available OS'es from GitHub Actions os matrix. You can't run tests in .NET Framework runtime on Ubuntu but on Windows you can.

@Corniel
Copy link
Author

Corniel commented Dec 25, 2024

It's simple - packaging and/or testing on available OS'es from GitHub Actions os matrix. You can't run tests in .NET Framework runtime on Ubuntu but on Windows you can.

Fair point.

I'm still not sure what the best solution should be (recommended by the rule):

<PropertyGroup Condition="'$(OS)' == 'Windows_NT'">
    <TargetFrameworks>net8.0;netstandard</TargetFrameworks>
<PropertyGroup>
<PropertyGroup Condition="'$(OS)' != 'Windows_NT'">
    <TargetFramework>net8.0</TargetFramework>
</PropertyGroup>

Using a choose when:

<Choose>
  <When Condition="'$(OS)' == 'Windows_NT'">
    <PropertyGroup>
        <TargetFrameworks>net8.0;netstandard</TargetFrameworks>
    </PropertyGroup>
  </When>
  <Otherwise>
    <PropertyGroup>
        <TargetFramework Condition="'$(OS)'  != 'Windows_NT'">net8.0</TargetFramework>
    </PropertyGroup>
  </Otherwise>
 </Choose>

Or as you did:

<PropertyGroup>
    <TargetFrameworks Condition="'$(OS)' == 'Windows_NT'">net8.0;netstandard</TargetFrameworks>
    <TargetFramework Condition="'$(OS)'  != 'Windows_NT'">net8.0</TargetFramework>
</PropertyGroup>

In this case it seems the lesser of the evils. But should I take that into account in the rule, and how?

@sungam3r
Copy link
Member

All variants are too verbose - less or more. I would completely ignore targetframework|s tags for this rule. I do not believe that anyone will bother changing well known simple approach living in codebases for ages.

@Corniel
Copy link
Author

Corniel commented Dec 27, 2024

All variants are too verbose - less or more. I would completely ignore targetframework|s tags for this rule. I do not believe that anyone will bother changing well known simple approach living in codebases for ages.

I agree, I've made a proposal to solve this FP: dotnet-project-file-analyzers/dotnet-project-file-analyzers#257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issue or pull request about performance problems tests Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants