Skip to content

fix: resolve SonarCloud findings across the codebase and enforce style rules#4990

Merged
arturcic merged 22 commits into
GitTools:mainfrom
arturcic:fix/sonar-fixes
Jun 29, 2026
Merged

fix: resolve SonarCloud findings across the codebase and enforce style rules#4990
arturcic merged 22 commits into
GitTools:mainfrom
arturcic:fix/sonar-fixes

Conversation

@arturcic

@arturcic arturcic commented Jun 29, 2026

Copy link
Copy Markdown
Member

Comprehensive SonarCloud cleanup across src, new-cli, and the build/ci solutions, plus two style-policy additions to .editorconfig. Code fixes are in this PR; analyzer false-positive / won't-fix dispositions were applied on SonarCloud directly.

Code fixes

GitVersion.Core

  • S1104 ×26 — public Options fields (GitVersionOptions, Settings, RepositoryInfo, …) encapsulated as auto-properties
  • S1006 ×9 — implementations now repeat the interface's default parameter values (TaggedSemanticVersionService, ConfigurationProvider)
  • S112 ×9 — throw InvalidOperationException / DirectoryNotFoundException instead of bare System.Exception (EffectiveConfiguration, GitPreparer)
  • S4035 ×4 — sealed the equality types SemanticVersion, SemanticVersionBuildMetaData, ReferenceName, NextVersion
  • S3267 ×6foreach + if simplified with LINQ Where/Select
  • S4136 ×5 — overloaded methods made adjacent
  • S3358 ×3 — nested ternaries extracted to if/else or locals
  • S3925 ×2 — dropped [Serializable] (GitVersionException, WarningException)
  • S1210 ×2 — comparison operators added to IComparable<T> types (ReferenceName, SemanticVersionWithTag)
  • S927 / S1066 / S3260 — parameter name aligned with base, nested if merged, private class sealed

Other projects

  • GitVersion.LibGit2Sharp — S112, S4136, S6608, S3220 (incl. fixing a latent Split('/', '\\') overload bug where '\\' bound to the int count parameter; now uses a cached char[] separator)
  • GitVersion.Configuration — S1006, S3267, S3220, S3925
  • GitVersion.App — S108, S4663
  • GitVersion.Output — S3267, S1450
  • GitVersion.BuildAgents — S112, S1125
  • new-cli (Cli.Generator) — S1125
  • build / ci — collection-expression conversions

Style policy (.editorconfig)

  • Enforce collection expressions (IDE0305) as error and apply across all solutions
  • Enforce braces on all control-flow statements (IDE0011) as error and apply across all solutions
  • Disable S1192 (duplicate string literals) in *.Tests projects — repeated literals there are intentional test data

Dispositions on SonarCloud (not code)

  • False Positive — analyzer limitations rather than real problems: C# 14 extension blocks; ArgumentParser S2259/S2325 (null-safe string? extension members; instance-method call); ConfigurationSerializer S2325 ×2 (interface implementations can't be static); TaggedSemanticVersionRepository S2589 ×3 (isCached flips inside the GetOrAdd factory); GitVersionCacheKeyFactory S3220 (collection-expression call is not actually ambiguous); JsonPropertyDescriptionAttribute S3903; GitVersionInformation template S1199
  • Won't Fix / Accept — S107 on DI constructors (parameter-object bundles intentionally avoided)

Out of scope (deferred)

  • S3776 cognitive complexity ×12
  • S1135 TODO comments ×4 (INFO)
  • Duplicated-lines metric on test projects — handled via SonarCloud Duplication Exclusions in project settings

Verification

  • dotnet build — 0 warnings / 0 errors on src, new-cli, and build/CI solutions
  • dotnet format --verify-no-changes — clean on all three solutions
  • Tests green (two environment-sensitive integration tests — a wall-clock perf assertion and a live-clone dynamic-repo test — flake only under full-suite load and pass in isolation)

🤖 Generated with Claude Code

@arturcic
arturcic force-pushed the fix/sonar-fixes branch 2 times, most recently from 4505e75 to 0720553 Compare June 29, 2026 12:02
arturcic added a commit to arturcic/GitVersion that referenced this pull request Jun 29, 2026
- Revert the ConfigurationSerializer static conversion: making the methods static
  required explicit interface implementations, which Sonar then flagged for S2325
  anyway (an explicit interface impl cannot be static). S2325 on IConfigurationSerializer
  implementations is not actionable and is dispositioned as False Positive instead.
  Restores ConfigurationHelper, WorkflowManager and the test call sites to their original form.
- GitRepositoryInfo: pass separators directly to Split's params overload instead of an
  array literal (S3878), keeping the [^1] indexing that resolved S6608.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
arturcic and others added 16 commits June 29, 2026 16:15
Resolve Sonar issue by replacing inline Regex compilation with a centralized cache to improve performance and resource usage.
Reorder declarations so method overloads sit together (no behavior change):
- Equals overloads in SemanticVersion, ReferenceName, and the LibGit2Sharp
  Branch/Commit/RefSpec/Remote/Tag wrappers
- Exclude overloads in PathFilter
- FindMergeBase overloads in RepositoryStore
- NormalizeGitDirectory overloads in GitPreparer

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- S112: throw InvalidOperationException instead of NullReferenceException
  (repo not initialized) and bare Exception (remote 401/403/404/unknown)
- S6608/S3220: use Split(['/','\\'])[^1] instead of Split('/','\\').Last()

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- S112: throw InvalidOperationException instead of bare Exception when the
  APPVEYOR_API_URL env var is missing (AppVeyor)
- S1125: '== true' -> 'is true' on nullable bool (AzurePipelines)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- S131: add default case (*) to the test shell scripts' argument parsing
- S4663: replace empty comment with rationale in MsBuildAppender's catch
- S1118: make the integration test Program class static

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Create the log file via CreateText().Dispose() instead of an empty using block (S108)
- Replace the empty comment in the log-write catch with the rationale for swallowing (S4663)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- ConfigurationProvider: default the Provide override parameter to null (S1006) and
  filter overrides with OfType instead of a foreach + null check (S3267)
- ConfigurationSerializer: make Serialize/Deserialize/ReadConfiguration static with
  explicit interface implementations (S2325); update call sites in ConfigurationHelper,
  WorkflowManager and tests
- BranchConfigurationBuilder: assign directly instead of calling the params overload (S3220)
- ConfigurationException: drop [Serializable] (S3925)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- ProjectFileUpdater: fold projectFile.FullName into a Select instead of a per-iteration local (S3267)
- WixVersionFileUpdater: make wixVersionFile a local variable instead of a field (S1450)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the AttributeClass?.TypeArguments.Any() is true check with a
null-safe { Length: > 0 } property pattern, removing the boolean literal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert the public fields on the Options DTOs (GitVersionOptions, AssemblySettingsInfo,
ConfigurationInfo, RepositoryInfo, Settings, WixInfo) to auto-properties, matching their
'Gets or sets' documentation, and drop two redundant '= false' initializers. PublicAPI
baseline updated to the corresponding get/set entries.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…VersionService (S1006)

Add the '= null' defaults declared on ITaggedSemanticVersionService to the four public
implementing methods so the implementation and interface agree.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rename the object Equals override parameter from 'other' to 'obj' to match
System.Object.Equals, updating the PublicAPI baseline accordingly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- SourceBranchFinder: seal the private SourceBranchPredicate class (S3260)
- Simplify foreach+if loops with LINQ Where/Select (S3267) in RepositoryStore,
  TaggedSemanticVersionService, VersionFieldExtensions and VersionInBranchNameVersionStrategy
- Add comparison operators to ReferenceName and SemanticVersionWithTag, which implement
  IComparable<T> (S1210); PublicAPI baseline updated

S3220 in GitVersionCacheKeyFactory is dispositioned as False Positive (the collection
expression call is not actually ambiguous); S4136 in this project was already resolved.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Normalize the casing of diagnostic IDs for consistency and set IDE0305 (collection expressions) to error severity.
- EffectiveConfiguration: InvalidOperationException for the 'should not happen' configuration
  invariant checks
- GitPreparer: DirectoryNotFoundException when the .git directory can't be found;
  InvalidOperationException for invalid dynamic-repository configuration

Also simplify a VersionStrategiesConverter array creation to a collection expression
(IDE0305) which surfaced in the build.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Seal SemanticVersion, SemanticVersionBuildMetaData, ReferenceName and NextVersion. They
override Equals/operators and implement IEquatable<T>; none are subclassed, so sealing
avoids the subclass-equality pitfall S4035 warns about.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
arturcic and others added 5 commits June 29, 2026 16:21
Apply the repo's new collection-expression rule to the build/ci solution:
PackageChocolatey, ContextExtensions and DockerContextExtensions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop [Serializable] from GitVersionException and WarningException (S3925)
- Replace nested ternaries with if/else or extracted locals in GitPreparer,
  SemanticVersionPreReleaseTag and MainlineVersionStrategy (S3358)
- Merge nested if in GitPreparer (S1066)

S2589 x3 in TaggedSemanticVersionRepository dispositioned False Positive (isCached flips
to false inside the GetOrAdd factory on a cache miss; Sonar can't model that). S107 on
the GitPreparer constructor accepted (DI constructor; parameter-object bundles declined).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update .editorconfig to require braces for all code blocks (csharp_prefer_braces = true) with error severity to address Sonar linting issues.
Set csharp_prefer_braces = true:error in .editorconfig and add braces to all
if/else/for/foreach/while/using statements across the src, new-cli and ci solutions
via dotnet format. Purely syntactic; no behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Repeated literals in the test scenarios (branch names, versions, config keys) are
intentional test data; suppress the rule for *.Tests projects via editorconfig.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
arturcic added a commit to arturcic/GitVersion that referenced this pull request Jun 29, 2026
- GitRepositoryInfo: Split('/', '\\') bound to Split(char, int count) because '\\'
  implicitly converts to int (count=92), so it split by '/' only instead of both
  separators. Use a cached char[] separator field so it binds to Split(params char[])
  unambiguously (fixes the latent bug and S3220).
- AzurePipelines: replace '?.StartsWith(...) is true' with an explicit null check to
  drop the redundant boolean literal (S1125).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- GitRepositoryInfo: Split('/', '\\') bound to Split(char, int count) because '\\'
  implicitly converts to int (count=92), so it split by '/' only instead of both
  separators. Use a cached char[] separator field so it binds to Split(params char[])
  unambiguously (fixes the latent bug and S3220).
- AzurePipelines: replace '?.StartsWith(...) is true' with an explicit null check to
  drop the redundant boolean literal (S1125).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@arturcic arturcic changed the title fix: resolve more SonarCloud issues in src (overloads, LibGit2Sharp, BuildAgents) fix: resolve SonarCloud findings across the codebase and enforce style rules Jun 29, 2026
@arturcic arturcic added this to the 6.x milestone Jun 29, 2026
@arturcic
arturcic merged commit 731b0ce into GitTools:main Jun 29, 2026
120 checks passed
@arturcic
arturcic deleted the fix/sonar-fixes branch June 29, 2026 17:18
@mergify

mergify Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Thank you @arturcic for your contribution!

@sonarqubecloud

Copy link
Copy Markdown

arturcic added a commit that referenced this pull request Jun 29, 2026
…e rules (#4990)

* fix(src): use Regex cache in PathFilter

Resolve Sonar issue by replacing inline Regex compilation with a centralized cache to improve performance and resource usage.

* fix(src): make overloaded methods adjacent (S4136)

Reorder declarations so method overloads sit together (no behavior change):
- Equals overloads in SemanticVersion, ReferenceName, and the LibGit2Sharp
  Branch/Commit/RefSpec/Remote/Tag wrappers
- Exclude overloads in PathFilter
- FindMergeBase overloads in RepositoryStore
- NormalizeGitDirectory overloads in GitPreparer

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(src): resolve LibGit2Sharp Sonar issues

- S112: throw InvalidOperationException instead of NullReferenceException
  (repo not initialized) and bare Exception (remote 401/403/404/unknown)
- S6608/S3220: use Split(['/','\\'])[^1] instead of Split('/','\\').Last()

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(src): resolve BuildAgents Sonar issues (S112, S1125)

- S112: throw InvalidOperationException instead of bare Exception when the
  APPVEYOR_API_URL env var is missing (AppVeyor)
- S1125: '== true' -> 'is true' on nullable bool (AzurePipelines)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: resolve assorted Sonar issues

- S131: add default case (*) to the test shell scripts' argument parsing
- S4663: replace empty comment with rationale in MsBuildAppender's catch
- S1118: make the integration test Program class static

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(app): resolve FileAppender Sonar issues (S108, S4663)

- Create the log file via CreateText().Dispose() instead of an empty using block (S108)
- Replace the empty comment in the log-write catch with the rationale for swallowing (S4663)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(config): resolve assorted Sonar issues

- ConfigurationProvider: default the Provide override parameter to null (S1006) and
  filter overrides with OfType instead of a foreach + null check (S3267)
- ConfigurationSerializer: make Serialize/Deserialize/ReadConfiguration static with
  explicit interface implementations (S2325); update call sites in ConfigurationHelper,
  WorkflowManager and tests
- BranchConfigurationBuilder: assign directly instead of calling the params overload (S3220)
- ConfigurationException: drop [Serializable] (S3925)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(output): resolve Sonar issues (S3267, S1450)

- ProjectFileUpdater: fold projectFile.FullName into a Select instead of a per-iteration local (S3267)
- WixVersionFileUpdater: make wixVersionFile a local variable instead of a field (S1450)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(new-cli): resolve CommandBaseGenerator Sonar issue (S1125)

Replace the AttributeClass?.TypeArguments.Any() is true check with a
null-safe { Length: > 0 } property pattern, removing the boolean literal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(core): encapsulate public Options fields as properties (S1104)

Convert the public fields on the Options DTOs (GitVersionOptions, AssemblySettingsInfo,
ConfigurationInfo, RepositoryInfo, Settings, WixInfo) to auto-properties, matching their
'Gets or sets' documentation, and drop two redundant '= false' initializers. PublicAPI
baseline updated to the corresponding get/set entries.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(core): match interface default parameter values in TaggedSemanticVersionService (S1006)

Add the '= null' defaults declared on ITaggedSemanticVersionService to the four public
implementing methods so the implementation and interface agree.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(core): rename NextVersion.Equals parameter to match base (S927)

Rename the object Equals override parameter from 'other' to 'obj' to match
System.Object.Equals, updating the PublicAPI baseline accordingly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(core): resolve MINOR Sonar issues (S3260, S3267, S1210)

- SourceBranchFinder: seal the private SourceBranchPredicate class (S3260)
- Simplify foreach+if loops with LINQ Where/Select (S3267) in RepositoryStore,
  TaggedSemanticVersionService, VersionFieldExtensions and VersionInBranchNameVersionStrategy
- Add comparison operators to ReferenceName and SemanticVersionWithTag, which implement
  IComparable<T> (S1210); PublicAPI baseline updated

S3220 in GitVersionCacheKeyFactory is dispositioned as False Positive (the collection
expression call is not actually ambiguous); S4136 in this project was already resolved.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* style: update editorconfig rules and enforce collection expressions

Normalize the casing of diagnostic IDs for consistency and set IDE0305 (collection expressions) to error severity.

* fix(core): throw specific exceptions instead of System.Exception (S112)

- EffectiveConfiguration: InvalidOperationException for the 'should not happen' configuration
  invariant checks
- GitPreparer: DirectoryNotFoundException when the .git directory can't be found;
  InvalidOperationException for invalid dynamic-repository configuration

Also simplify a VersionStrategiesConverter array creation to a collection expression
(IDE0305) which surfaced in the build.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(core): seal equality types (S4035)

Seal SemanticVersion, SemanticVersionBuildMetaData, ReferenceName and NextVersion. They
override Equals/operators and implement IEquatable<T>; none are subclassed, so sealing
avoids the subclass-equality pitfall S4035 warns about.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* style(build): use collection expressions in ci solution (IDE0305)

Apply the repo's new collection-expression rule to the build/ci solution:
PackageChocolatey, ContextExtensions and DockerContextExtensions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(core): resolve MAJOR Sonar issues (S3925, S3358, S1066)

- Drop [Serializable] from GitVersionException and WarningException (S3925)
- Replace nested ternaries with if/else or extracted locals in GitPreparer,
  SemanticVersionPreReleaseTag and MainlineVersionStrategy (S3358)
- Merge nested if in GitPreparer (S1066)

S2589 x3 in TaggedSemanticVersionRepository dispositioned False Positive (isCached flips
to false inside the GetOrAdd factory on a cache miss; Sonar can't model that). S107 on
the GitPreparer constructor accepted (DI constructor; parameter-object bundles declined).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* style: enforce braces for all control flow statements

Update .editorconfig to require braces for all code blocks (csharp_prefer_braces = true) with error severity to address Sonar linting issues.

* style: enforce braces on control-flow statements (IDE0011)

Set csharp_prefer_braces = true:error in .editorconfig and add braces to all
if/else/for/foreach/while/using statements across the src, new-cli and ci solutions
via dotnet format. Purely syntactic; no behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* style: disable S1192 (duplicate string literals) in test projects

Repeated literals in the test scenarios (branch names, versions, config keys) are
intentional test data; suppress the rule for *.Tests projects via editorconfig.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: resolve remaining PR #4990 Sonar issues (S3220, S1125)

- GitRepositoryInfo: Split('/', '\\') bound to Split(char, int count) because '\\'
  implicitly converts to int (count=92), so it split by '/' only instead of both
  separators. Use a cached char[] separator field so it binds to Split(params char[])
  unambiguously (fixes the latent bug and S3220).
- AzurePipelines: replace '?.StartsWith(...) is true' with an explicit null check to
  drop the redundant boolean literal (S1125).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@arturcic arturcic modified the milestones: 6.x, 6.8.0 Jun 30, 2026
@arturcic

Copy link
Copy Markdown
Member Author

🎉 This issue has been resolved in version 6.8.0 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant