Fix 20 more SonarCloud issues; document 8 multi-target false positives - #158
Merged
Conversation
- S2223 (3, HIGH): PropertyTemplate.AutoGet/AutoSet/AutoInit were mutable
public static fields. Made static readonly - they are compared by
reference in WriteTo to detect auto-property shorthand, so they must stay
single fixed instances and readonly enforces that without changing the
comparison.
- S1192 (4): extracted the repeated validation failure message in the
kebab/macro/snake/sentence case attributes to a FailureMessage constant.
- S6610 (6): StartsWith("-")/EndsWith("_") -> the char overloads in the
same three casing attributes. Verified across all TFMs; Polyfill supplies
the char overloads on netstandard2.0.
- S6580 (2): IsDateTime/IsTimeSpan now pass an explicit format provider.
Deliberately CultureInfo.CurrentCulture, not InvariantCulture: that is
what the provider-less overload already used, so which strings validate
is unchanged. Switching to invariant would be a behaviour change and is
the maintainers call.
- S3358 (3): extracted the nested ternary in AccessibilityLevelAgainst into
named thresholds, and replaced the chained Clamp01 ternaries in Color and
Hsl with Math.Clamp (identical NaN behaviour).
Also refreshed CLAUDE.md - it still documented the old three-line header.
Carried over from the superseded #151, extended to note that
SourceGeneratorTests now asserts the emitted header.
Build clean on all TFMs, 1089/1089 tests pass.
The explicit IRelativePath.AsAbsolute() implementation sat between the two public AsAbsolute overloads, splitting the group. Moved it below both in RelativeFilePath and RelativeDirectoryPath. Pure reordering, no behaviour change. Build clean, 1089/1089 tests pass.
Addresses both quality-gate failures on this PR. new_duplicated_lines_density (38.7%): the FailureMessage constants were introduced with an identical three-line XML doc comment in four files, which is literal duplication. The comment is gone; a private const does not need one, and the declaration line itself differs per file. new_coverage (56.3%): PropertyTemplate had no tests at all. Added four covering auto-get/auto-set, auto-get/auto-init, the abstract (no accessor) form, and a custom getter. This needed InternalsVisibleTo on Semantics.SourceGenerators, matching the pattern already used by Semantics.Paths and Semantics.Quantities. Worth a look: AutoGet, AutoSet and AutoInit are not referenced by any generator, so the auto-property shorthand branches in WriteTo are currently unreachable from production code. They are kept and now exercised rather than deleted, since they read as a deliberate affordance for template authors - but if they are not wanted, deleting them would also remove the dead branch and cut WriteTo s S3776 complexity. SourceGeneratorTests now iterates the generators inside one test instead of using [DynamicData] with an IIncrementalGenerator parameter. A public test method taking a reference type trips CA1062, and every escape is blocked here: CA1510 rejects an explicit ArgumentNullException throw, CA1062 does not recognise ArgumentNullException.ThrowIfNull, and Polyfill s Ensure cannot be referenced because its shim types collide with the ones the generator assembly embeds publicly (106 x CS0433). Assertion messages name the generator, so failures stay unambiguous. ktsu.CodeBlocker no longer needs ExcludeAssets="compile" now that BundleAnalyzerDependencies=false stops the facade leak. Build clean, 1086/1086 tests pass.
Closes the remaining coverage gaps on this PR, all of which turned out to be genuinely untested branches rather than artefacts of the changes: - Each casing validator has two rejection branches beyond the leading and trailing separator checks - a separator belonging to a different convention, and correct separators with the wrong letter case. Neither was exercised for kebab, snake or macro, nor was sentence case s lowercase-first-letter branch. Covered by one table-driven test rather than four near-identical ones, which would have added duplication. - RelativeFilePath and RelativeDirectoryPath implement IRelativePath.AsAbsolute() explicitly so the interface returns the base AbsolutePath while the public overload returns the specific type. No test had ever gone through the interface, so both explicit implementations were dead to the suite. Build clean, 1088/1088 tests pass.
Merged main, which bumped ktsu.Sdk 2.26.1 -> 2.27.0. That build had not run on main yet (only the Dependabot workflow did), so these failures are the SDK bump rather than anything in this branch - main will hit them too. - KTSU0001 (new in 2.27.0): the downlevel targets must reference the packages supplying the framework types they use rather than picking them up transitively. Added System.Memory for netstandard2.1 (Paths, Strings and Strings.Identifiers already had it for netstandard2.0 only; Color and Music had neither) and System.Threading.Tasks.Extensions for netstandard2.0, across all five multi-targeted libraries. Added the missing System.Threading.Tasks.Extensions PackageVersion. - Committed the .editorconfig the SDK rewrites on build - 2.27.0 changed the C++ section to the brace-glob form. This is what failed the verify-generated job, not generator drift. Build clean on all TFMs, 1088/1088 tests pass.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #157. Fixes 20 more SonarCloud issues and documents 8 that are false positives on this codebase.
Fixed — 20
PropertyTemplate.AutoGet/AutoSet/AutoInitwere mutable public static fields →static readonlyFailureMessageconstantStartsWith("-")/EndsWith("_")→ thecharoverloadsAccessibilityLevelAgainst;Clamp01→Math.ClampIsDateTime/IsTimeSpannow pass an explicit format providerAsAbsoluteoverloads in the relative path typesNotes on two of those:
WriteToto detect auto-property shorthand, so they must remain single fixed instances.static readonlyenforces that without changing the comparison;constisn't possible for delegates.CultureInfo.CurrentCulture, notInvariantCulture. That is what the provider-less overload already used, so which strings validate is unchanged. Invariant is arguably the better semantic for a validation attribute (validation currently varies by machine locale), but that is a behaviour change and your call — happy to switch it.Also refreshed
CLAUDE.md, which still documented the old three-line header. Carried over from the superseded #151 (thanks — it was the clearest description of theCOPYRIGHT.md→file_header_template→WriteHeaderTochain anywhere in the repo), extended to note thatSourceGeneratorTestsnow asserts the emitted header so drift fails a test.Not fixed — 8 false positives, with reasons
S8969 ×4 — "remove this null-forgiving operator; the compiler already knows". Verified by removing all four and building: every one fails on netstandard2.0 with CS8604/CS8602.
string.IsNullOrEmptylacks[NotNullWhen(false)]there, so the!is mandatory — Sonar only analyses net10, where it is genuinely redundant. Same multi-target shape as the IDE0370 issue from the earlier SDK migration. Unfixable without dropping netstandard2.0 or adding conditional compilation, which the repo conventions discourage.S1244 ×4 — "do not check floating point equality with exact values".
Hsl.cs(×2):maxis assigned from one ofsrgb.R/G/B, somax == srgb.Ris an identity test, not an approximate one. An epsilon range would be actively wrong — it could match two channels and pick the wrong hue sector.NormalizedParameter.cs(×2):skew == 1.0is a fast-path skip, andMath.Pow(x, 1.0) == xregardless, so the comparison cannot change the result — only whether thePowcall happens.Verification
dotnet build— 0 errors, 0 warnings, all target frameworksdotnet test— 1089/1089 passStill open after this — ~66
S3776cognitive complexity ×9 (mostlyQuantitiesGenerator, now covered by tests so safer to refactor),S1133deprecation TODOs ×10,S1192×15 in the generators,S4136×8 inSemanticString.cs(pure reordering in a 600-line public API file with#ifblocks — high diff noise, deserves its own pass),S1172×6,S3267×6, plus a small tail.