Enforce IDE0005 during build by enabling GenerateDocumentationFile #5854

jgonz120 · 2024-06-14T18:03:57Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2845

Regression? Last working version:

Description

Updated two projects to include style rules enforcement during build, fixing the build errors for those two projects. Currently certain rules are not enforced unless XML documents are generated, dotnet/roslyn#41640.

PR Checklist

src/NuGet.Clients/NuGet.CommandLine/CommandLineMachineWideSettings.cs

src/NuGet.Core/NuGet.Versioning/NuGet.Versioning.csproj

zivkan

The PR checklist is not filled out, and personally I find the PR title misleading, as it's just enabling XMLDoc generation for two projects, but it's not enabling style rules more broadly (style rules other than XMLDoc comments to the two projects, and it's not clear to me why just these two projects, rather than most src/NuGet.Core/* projects)

build/common.targets

src/NuGet.Clients/NuGet.CommandLine/CommandLineMachineWideSettings.cs

src/NuGet.Core/NuGet.Versioning/NuGet.Versioning.csproj

src/NuGet.Core/NuGet.Versioning/VersionComparer.cs

src/NuGet.Core/NuGet.Versioning/VersionRangeComparer.cs

src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj

zivkan · 2024-06-22T23:46:15Z

src/NuGet.Core/NuGet.Versioning/NuGet.Versioning.csproj

@@ -7,6 +7,7 @@
    <PackProject>true</PackProject>
    <Shipping>true</Shipping>
    <IncludeInVSIX>true</IncludeInVSIX>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>


The linked issue isn't clearly defined, making it sound like the intention is to enforce all style rules, not just documentation files, and also across all projects, not just one or two. The PR description also doesn't explain either why only XMLDoc, or why only 1 or 2 projects.

Most of the src/NuGet.Core/* projects are nupkgs published to nuget.org. Do the other project already generate XMLDoc, and NuGet.Versioning was the only one that wasn't?

Should the linked issue be changed to explicitly mention it's only about XMLDoc, and not other style issues?

I agree with Andy that we should clarify the scope of the PR/issue.

Okay I updated the issue. I don't really have a reason why I picked these two projects, I just needed to start somewhere.

One thing though, a know that is set to true for projects with set to true

NuGet.Client/build/common.targets

Lines 39 to 42 in 15ad272

<PropertyGroup Condition=" '$(PackProject)' == 'true' ">

<GenerateDocumentationFile Condition=" '$(GenerateDocumentationFile)' == '' ">true</GenerateDocumentationFile>

<DocumentationFile Condition=" '$(DocumentationFile)' == '' AND '$(GenerateDocumentationFile)' == 'true' AND '$(IsNetCoreProject)' != 'true' ">$(OutputPath)\$(AssemblyName).xml</DocumentationFile>

</PropertyGroup>

Despite that I still don't get build errors for using statements until I specify GenerateDocumentationFile in the csproj.

can you upload a binlog for the project in question?
You can do it in teams if that's easier.

Okay I moved this pack from the common.targets to common.project.props, and that worked. Is there a better location for it? Basically, we need to set it in a different location because it's getting set by another config.

Keep in mind that the NuGet.Client repo has existed for years, before the .NET (Core) SDK was a thing, so while the SDK might now have defaults to make it work out of the box, perhaps in the past it didn't, or pre-SDK style projects might have worked differently as well.

I prefer a common config to apply to all projects, rather than opting individual projects into docs.

Moving the code to the common.project.props file doesn't work because when it goes to evaluate the condition is empty, so is set to false in Microsoft.NET.Sdk.BeforeCommon.targets.

Which condition is false?

This one, since PackProject is set later in the file it has no value when this is evaluated.

<PropertyGroup Condition=" '$(PackProject)' == 'true' ">

I prefer a common config to apply to all projects, rather than opting individual projects into docs.

Agreed but where that common config should live is difficult because of how <GenerateDocumentationFile> is set by Microsoft.NET.Sdk.BeforeCommon.targets.

Since we now know that without <GenerateDocumentationFile> we can't enforce all the style rules, is it worth just enabling it for everyone?

I think it's ok to enable it for everyone.
We'd probably need to disable the IDE0005 in some projects where it fails,.
I can't think of anything else.

@zivkan to focus this conversation here because this has the most context. So, the latest change I pushed is what it looks like if we enable GenerateDocumentationFile for all projects all at once. I kept flipping back and forth between enabling it per project then once all projects have had it enabled moving it to the central build props project.

Currently we do not generate the documentation file for any of our projects because of the way the value is determined. So, I believe options for moving forward are:

Enable GenerateDocumentationFile for all projects, using NoWarn to suppress build errors, then overtime remove the suppressions per project and fix the errors that come up. (current state)

Enable GenerateDocumentationFile for all projects, using WarningsNotAsErrors to suppress build errors, then overtime remove the suppressions per project and fix the errors that come up.

Enable GenerateDocumentationFile for individual projects, fixing the build errors as it is enabled. Once all projects have been updated to generate documentation files, we can move it to a central location.

Force GenerateDocumentationFile to true for projects which have PackProject set to true. This will limit the scope of how many projects need to be fixed upfront, however it will still be a lot. We would need to selectively turn on GenerateDocumentationFile for all projects in order to get the style fixes like IDE0005 to show up as errors in the build.

build/common.project.props

This reverts commit 4f91a03.

zivkan · 2024-07-30T07:08:31Z

src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj

@@ -24,7 +24,8 @@
    <UsePublicApiAnalyzer>false</UsePublicApiAnalyzer>
    <GetSigningInputsDependsOn>GetSigningInputsCustom</GetSigningInputsDependsOn>
    <GetSymbolsToIndexDependsOn>GetSymbolsToIndexCustom</GetSymbolsToIndexDependsOn>
-</PropertyGroup>
+    <NoWarn>$(NoWarn);CS1591;IDE0059;IDE1006;IDE0049</NoWarn>


CS1591 is related to XMLDoc, and therefore the title of this PR, but the other 3 codes are not.

We want to keep IDE1006, so disabling for the whole project feels wrong. I know we've got a bunch of noise on static properties, but we want to keep warnings (ideally enforce) public properties being Pascal Case, private members being _camelCase, methods being Pascal Case.

An in my opinion, IDE0059 seems like a good analyzer we should keep.

I think IDE0049 is very reasonable too, though we've got a lot of violations in our repo. But suppressing the code increases risk that future code changes will continue to add more violations, making it harder to enforce in the future.

zivkan · 2024-07-30T07:12:09Z

test/NuGet.Core.FuncTests/NuGet.Protocol.FuncTest/NuGet.Protocol.FuncTest.csproj

@@ -44,4 +45,4 @@
              ItemName="ReferencesCopiedInThisBuild" />
    </Copy>
  </Target>
-</Project>
+</Project>


let's keep trailing new line. I thought our editorconfig actually specifies this

zivkan · 2024-07-30T07:19:53Z

build/common.project.props

+  <!-- Write out .XML files for projects that will be packed. -->
+  <PropertyGroup>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <DocumentationFile Condition=" '$(DocumentationFile)' == '' AND '$(GenerateDocumentationFile)' == 'true' AND '$(IsNetCoreProject)' != 'true' ">$(OutputPath)\$(AssemblyName).xml</DocumentationFile>


I think it's a great thing to enforce XMLDoc for the packages we publish to nuget.org. However,

This issue is titled enforce IDE0005, but the docs say IDE0005 is about removing unnecessary usings, which has nothing to do with XMLDoc, and therefore the title "enforce IDE005 by enabling GenerateDocumentationFile" doesn't make sense

I don't see anything in this PR that changes enforcement of IDE005. Our CI builds will continue to pass after this merges if people add new violations in future PRs.

To be honest, I'm not a fan of adding NoWarn. I see NoWarn as tech debt, so now that I see the consequence of enabling GenerateDocumentationFile to all projects in the solution, I prefer the old implementation where it's only selectively enabled for projects that are packed.

zivkan · 2024-07-30T07:22:22Z

src/NuGet.Clients/NuGet.Console/NuGet.Console.csproj

@@ -9,6 +9,7 @@
    <TargetFramework>$(NETFXTargetFramework)</TargetFramework>
    <UseWpf>true</UseWpf>
    <ExcludeFromSourceOnlyBuild>true</ExcludeFromSourceOnlyBuild>
+    <NoWarn>$(NoWarn);CS1572;CS1573;CS1591;IDE0005;IDE1006</NoWarn>


The PR title says it's enforcing IDE0005, but it's just being suppressed here, not enforced.

zivkan · 2024-07-30T07:23:57Z

test/NuGet.Tests.Apex/NuGet.OptProf/NuGet.OptProf.csproj

-                        $(PkgMicrosoft_Test_Apex_VisualStudio)\lib\net46\Microsoft.Test.Apex.RemoteCodeInjector.dll;
-                        $(PkgMicrosoft_Test_Apex_VisualStudio)\lib\net46\Microsoft.Test.Apex.RemoteCodeInjector.x64.dll"
-               Name="%(Filename)" />
+    <Reference Include="$(PkgMicrosoft_Test_Apex_VisualStudio)\lib\net46\*.dll" Exclude="$(PkgMicrosoft_Test_Apex_VisualStudio)\lib\net46\Microsoft.Test.Apex.PrismIntegration.dll;&#xD;&#xA;                        $(PkgMicrosoft_Test_Apex_VisualStudio)\lib\net46\Microsoft.Test.Apex.RemoteCodeInjector.dll;&#xD;&#xA;                        $(PkgMicrosoft_Test_Apex_VisualStudio)\lib\net46\Microsoft.Test.Apex.RemoteCodeInjector.x64.dll" Name="%(Filename)" />


This looks like a bad change.

dotnet-policy-service · 2024-08-06T22:29:21Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

jgonz120 requested a review from a team as a code owner June 14, 2024 18:03

jgonz120 commented Jun 14, 2024

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/CommandLineMachineWideSettings.cs Outdated Show resolved Hide resolved

jgonz120 commented Jun 14, 2024

View reviewed changes

src/NuGet.Core/NuGet.Versioning/NuGet.Versioning.csproj Outdated Show resolved Hide resolved

zivkan reviewed Jun 14, 2024

View reviewed changes

jgonz120 changed the title ~~Enforce style rules during build~~ Enforce style rules during build by enabling GenerateDocumentationFile Jun 14, 2024

zivkan previously approved these changes Jun 18, 2024

View reviewed changes

jgonz120 dismissed zivkan’s stale review via 09a6499 June 18, 2024 23:40

jeffkl added the Engineering Changes related to the build infrastructure of the repo and that do not change product code label Jun 20, 2024

jgonz120 and others added 14 commits June 20, 2024 14:29

enforce rules for a project

334fa18

add change to csproj to break build

9fb5e25

Fix build errors

3d68c58

removed supressing from common targets and supressed it per file.

25ed60c

revert editor config

743a1ad

Removed EnforceCodeStyleInBuild

823993b

Added generatedoc file to true

4262627

Update common.targets

21af9ed

remove supression

21df0c6

update comment again

98d8438

remove extra line

a9c2f16

enable GenerateDocumentationFile

f84c641

Add .editorconfig

98c68c4

moved to a no warn in csproj

b3f9a76

jgonz120 force-pushed the dev-jgonz120-BuildStyles branch from 249a398 to b3f9a76 Compare June 20, 2024 21:30

jgonz120 requested review from nkolev92 and zivkan June 21, 2024 18:09

nkolev92 reviewed Jun 21, 2024

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj Outdated Show resolved Hide resolved

zivkan reviewed Jun 22, 2024

View reviewed changes

Undo CS1591 comment

d12aba3

jgonz120 changed the title ~~Enforce style rules during build by enabling GenerateDocumentationFile~~ Enforce IDE0005 during build by enabling GenerateDocumentationFile Jul 1, 2024

jgonz120 closed this Jul 2, 2024

jgonz120 reopened this Jul 2, 2024

jgonz120 marked this pull request as draft July 2, 2024 22:13

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 9, 2024

dotnet-policy-service bot closed this Jul 17, 2024

jgonz120 reopened this Jul 22, 2024

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 22, 2024

moved where GenerateDocumentationFile is being set

4f91a03

jgonz120 marked this pull request as ready for review July 22, 2024 22:37

jgonz120 requested review from nkolev92 and zivkan July 23, 2024 17:24

nkolev92 previously approved these changes Jul 23, 2024

View reviewed changes

zivkan reviewed Jul 25, 2024

View reviewed changes

build/common.project.props Outdated Show resolved Hide resolved

Revert "moved where GenerateDocumentationFile is being set"

77ef2cc

This reverts commit 4f91a03.

jgonz120 dismissed nkolev92’s stale review via 77ef2cc July 25, 2024 22:32

enable generate docs for all projects and supress warnings

fceda86

zivkan reviewed Jul 30, 2024

View reviewed changes

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 6, 2024

dotnet-policy-service bot closed this Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce IDE0005 during build by enabling GenerateDocumentationFile #5854

Enforce IDE0005 during build by enabling GenerateDocumentationFile #5854

jgonz120 commented Jun 14, 2024 •

edited

Loading

zivkan left a comment

zivkan Jun 22, 2024

nkolev92 Jul 1, 2024

jgonz120 Jul 1, 2024 •

edited

Loading

nkolev92 Jul 1, 2024

jgonz120 Jul 22, 2024

zivkan Jul 25, 2024

jgonz120 Jul 25, 2024 •

edited

Loading

jgonz120 Jul 25, 2024

nkolev92 Jul 26, 2024

jgonz120 Jul 30, 2024

zivkan Jul 30, 2024

zivkan Jul 30, 2024

zivkan Jul 30, 2024

zivkan Jul 30, 2024

zivkan Jul 30, 2024

dotnet-policy-service bot commented Aug 6, 2024

	<PropertyGroup Condition=" '$(PackProject)' == 'true' ">
	<GenerateDocumentationFile Condition=" '$(GenerateDocumentationFile)' == '' ">true</GenerateDocumentationFile>
	<DocumentationFile Condition=" '$(DocumentationFile)' == '' AND '$(GenerateDocumentationFile)' == 'true' AND '$(IsNetCoreProject)' != 'true' ">$(OutputPath)\$(AssemblyName).xml</DocumentationFile>
	</PropertyGroup>

Enforce IDE0005 during build by enabling GenerateDocumentationFile #5854

Enforce IDE0005 during build by enabling GenerateDocumentationFile #5854

Conversation

jgonz120 commented Jun 14, 2024 • edited Loading

Bug

Description

PR Checklist

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgonz120 Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgonz120 Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnet-policy-service bot commented Aug 6, 2024

jgonz120 commented Jun 14, 2024 •

edited

Loading

jgonz120 Jul 1, 2024 •

edited

Loading

jgonz120 Jul 25, 2024 •

edited

Loading