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

Update code to use 'var' where type is explicit. #2817

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Conversation

michaelcfanning
Copy link
Member

@michaelcfanning michaelcfanning commented Jul 24, 2024

No behavioral changes, hence no release notes.

This change also disables automated SARIF object model generation: this code is stable and the autogenerator emit doesn't conform currently to our style guidelines. To enforce them we therefore need to check in and update a stable copy.

Finally, we now enforce strict adherence to the guidelines by elevating problems to errors via CLI build.

Copy link
Member

@rwoll rwoll left a comment

Choose a reason for hiding this comment

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

stamped with a nice-to-have comment

@@ -50,7 +50,7 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm

LogicalLocations.Clear();

XmlReaderSettings settings = new XmlReaderSettings
var settings = new XmlReaderSettings
Copy link
Member

Choose a reason for hiding this comment

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

I see csharp_style_var_when_type_is_apparent = true:error in the .editor config (

csharp_style_var_when_type_is_apparent = true:error
), but PRs prior to this are not failing the style check.

While this PR is a great cleanup, unless it's enforced in the PR check (https://github.com/microsoft/sarif-sdk/pull/2817/checks), the inconsistent style is going to creep back in.

This is likely due to EnforceCodeStyleInBuild missing in csproj/props: https://learn.microsoft.com/en-us/community/content/how-to-enforce-dotnet-format-using-editorconfig-github-actions

Consider fixing that in this PR, too.

@@ -94,7 +94,7 @@ function Invoke-DotNetBuild($solutionFileRelativePath) {
Write-Information "Building $solutionFileRelativePath..."

$solutionFilePath = Join-Path $SourceRoot $solutionFileRelativePath
& dotnet build $solutionFilePath --configuration $Configuration --verbosity $BuildVerbosity --no-incremental -bl -p:WarningsAsErrors="MSB3277"
& dotnet build $solutionFilePath --configuration $Configuration --verbosity $BuildVerbosity --no-incremental -bl -p:WarningsAsErrors="MSB3277" /p:EnforceCodeStyleInBuild=true
Copy link
Member Author

Choose a reason for hiding this comment

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

EnforceCodeStyleInBuild

Internet query informed me that EnforceCodeStyleInBuild as expressed in csproj files only works within the IDE. For CLI builds you need to explicitly set this property (which appears to be a bit of a hammer, i.e., it will be applied everywhere. Fortunately that's what we want).

@@ -297,6 +297,7 @@ csharp_style_prefer_local_over_anonymous_function = true:suggestion
dotnet_diagnostic.SA1602.severity = suggestion
dotnet_diagnostic.SA1307.severity = silent
csharp_style_implicit_object_creation_when_type_is_apparent = true:suggestion
csharp_style_prefer_primary_constructors = true:suggestion
Copy link
Member Author

Choose a reason for hiding this comment

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

true

interestingly, this item was added when I explicitly used 'dotnet format' to get all autofixes I could from the command-line. This must be a tool feature, to add new diagnostics as they are added to the toolchain.

@@ -51,7 +51,7 @@ public LineInfo(int startOffset, int lineNumber)
/// <returns>true if the objects are considered equal, false if they are not.</returns>
public override bool Equals(object obj)
{
LineInfo? other = obj as LineInfo?;
Copy link
Member Author

Choose a reason for hiding this comment

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

LineInfo

here it is friends, the application of our var standard. we do not require the type to be expressed if the type is already quite explicit. we do this to reduce the # of chars on a line to and allow left-aligning var declarations nicely for this circumstance.


<Import Project="ToDotNet\ToDotNet.targets" />

<!-- Disabling the automatic code generation. This code emit is generating
Copy link
Member Author

Choose a reason for hiding this comment

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

Disabling

Everyone: please note this change. The SARIF OM has been stable for years now. We don't need to keep updating it. We can't enforce stringent style guidelines on that code if the autogen keeps overwriting changes again (and I don't feel like investing in fixing the JSchema codegen for these style issues, though that would be nice if someone did it).

@@ -1,7 +1,9 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#if DEBUG
Copy link
Member Author

Choose a reason for hiding this comment

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

DEBUG

Please note this distinction you may encounter, I typically build as DEBUG in the IDE, our command-line is release. And so you may see discrepancies in style issues between IDE and command-line builds.

@michaelcfanning michaelcfanning merged commit 03b4ef4 into main Jul 24, 2024
8 checks passed
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.

3 participants