Skip to content

Conversation

@rossgrambo
Copy link
Member

@rossgrambo rossgrambo commented Apr 23, 2024

Why this PR?

What was done?

  1. Added a .editorconfig file to the solution
    a. This is modified from the default .editorconfig generated by Visual Studio.
  2. Ran dotnet format
  3. Ran Visual Studio .NET code analyzer & manually resolved issues
  4. Updated FeatureManagement and FeatureManagement.AspNetCore with EnforceCodeStyleInBuild: true. This means warnings will show in a build when the code style is not being followed.

Adding an .editorconfig enables code formatting and code analyzing tools to work. Simply by adding this file, Visual Studio and other IDEs will now respect the style choices for this project when making changes within it. Additionally, this unlocks GitHub actions that can evaluate code style and formatting and catch small mistakes for us.

Visible Changes

No visible changes

Philosophy

Code styles should never feel like a hurdle or a headache that we need to adhere to- so at no point should these changes block or slow development. However I believe adding this file allows for a clear definition of rules, a clear place to change style choices as we see fit, and enables IDEs & tooling to quickly follow suit.

This also means all other PRs will be cleaner- as any code style issues will already be caught and handled by tooling.

@zhiyuanliang-ms
Copy link
Member

This PR is great! This repo has been missing lint checks for a long time. The format check should be added to the azure pipeline.

/// A disabled feature handler that wraps an inline handler.
/// </summary>
class InlineDisabledFeaturesHandler : IDisabledFeaturesHandler
sealed class InlineDisabledFeaturesHandler : IDisabledFeaturesHandler
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change.

Copy link
Member Author

@rossgrambo rossgrambo Apr 25, 2024

Choose a reason for hiding this comment

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

I recalled someone saying sealing classes is a breaking change, but the warning message convinced me it was okay in this case:

CA1852 Type 'InlineDisabledFeaturesHandler' can be sealed because it has no subtypes in its containing assembly and is not externally visible

If we think it's still breaking- or just don't want to risk it I can disable this rule.

Copy link
Member

Choose a reason for hiding this comment

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

Oh it's not a public type. Then indeed it can be sealed without a breaking change.

}
ArgumentNullException.ThrowIfNull(app);

if (string.IsNullOrEmpty(featureName))
Copy link
Member

Choose a reason for hiding this comment

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

Confused why it would suggest Argument.ThrowIfNull for null checks, but not ThrowIfNullOrEmpty for string checks.

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 had the same thought. Turns out ThrowIfNull was added in .NET 6 while ThrowIfNullOrEmpty was added in .NET 7. And we still have .NET 6 as a target.

@jimmyca15
Copy link
Member

This is the type of change that is cross-repo. If we were to adopt this, all repos in our scope should adopt it.

@zhiyuanliang-ms zhiyuanliang-ms self-assigned this May 27, 2024
@zhiyuanliang-ms
Copy link
Member

Adjusted the AnalysisLevel in csproj file to suppress CA1848 and CA2254 warnings:
image

Now, the AnalysisLevel for Microsoft.FeatureManagement is 5.0 because Microsoft.FeatureManagement is targeted on netstandard and 5.0 is the lowest AnalysisLevel which was available for the .NET 5 release is used.

the AnalysisLevel for Microsoft.FeatureManagement.AspNetCore is 6.0 because Microsoft.FeatureManagement.AspNetCore is targeted on .NET 6.0.

@zhiyuanliang-ms
Copy link
Member

zhiyuanliang-ms commented May 28, 2024

A reference for all the format rules:
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/csharp-formatting-options

@zhiyuanliang-ms
Copy link
Member

Move to #472

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.

4 participants