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

Enforce style rules during build #5796

Closed
wants to merge 5 commits into from
Closed

Conversation

jgonz120
Copy link
Contributor

Bug

Fixes:

Regression? Last working version:

Description

Updated two projects to include style rules enforcement during build, fixing the build errors for those two projects.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

build/common.targets Outdated Show resolved Hide resolved
@@ -8,17 +8,23 @@
namespace NuGet.CommandLine
{
[Export(typeof(Configuration.IMachineWideSettings))]
#pragma warning disable CS1591 // Missing XML comment for publicly visible type or member
Copy link
Member

Choose a reason for hiding this comment

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

We've historically added our suppressions in a GlobalSuppressions file, since this makes the file really busy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't give me the option to in the "Quick actions" even after creating a global suppression file. Lemme see just force the value in there and see if it works

Copy link
Member

Choose a reason for hiding this comment

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

I 100% disagree with putting all the suppressions in a GlobalSuppressions file. It being busy in the code is a feature, because when I'm reviewing a future PR and I see they're modifying a line within a #pragma disable block, or a line right next to it, I can comment on the PR saying please remove this tech debt. If the suppression is in the GlobalSuppresions file, it becomes much more difficult to know that there's an analyzer warning that's being suppressed and that improvements could be made.

If this is specifically about VS1591 or other XML comment warnings, then maybe we should disable this analyzer for proejcts that don't ship to nuget.org as packages.

I just don't see GlobalSuppressions.cs as a positive thing, with the possible exception of WPF code and the "keep the object heirarchy lower than X" warning.

@@ -7,6 +7,9 @@
<PackProject>true</PackProject>
<Shipping>true</Shipping>
<IncludeInVSIX>true</IncludeInVSIX>
<EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
<GenerateDocumentationFile Condition="'$(IsOfficialBuild)' == 'true'">false</GenerateDocumentationFile>
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this cause it to only fail in the official build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the opposite, right now the documents are only generated if the project has "PackProject" is set to true, and there's nothing set in "GenerateDocumentationFile".

I wanted to make sure that nothing changes for the official builds. Without the Generated documentation files, some of the rules don't get enforced.

Copy link
Member

Choose a reason for hiding this comment

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

I might be missing some other motivation here, but I tend to be of the opinion that build rules should remain the same for simplicity.

There's no harm in getting xml files for projects we don't ship as packages, as we have other logic (such as VSIX), to ensure we only ship dlls.

@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity No recent activity. label May 25, 2024
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:No recent activity No recent activity.
Projects
None yet
3 participants