Address the SonarCloud findings from #57 - #58
Merged
Conversation
The quality gate passed on #57 and it merged before these landed, so all 24 findings are in code that is now on main. One of them is a real defect rather than a style note. S2699, reported as a blocker, was right: WhatThePaletteCreates_IsReadyToConnect reads every slot of every new node and asserts nothing about what comes back. It also has no guard against its own filter matching nothing, so it would pass just as happily if the palette stopped offering these nodes entirely — which is the regression it exists to catch. It now asserts that all three templates are found, and that every operand already sitting in a slot is the placeholder the rest of the library understands rather than a null it does not. S1192 asked for constants where a literal repeats. AstSchema already keeps one for the arguments slot's name and the new slots now have theirs; the palette's category names and the serializer's expectedType key follow the same convention their own files already set. The MSTest analyzers asked for the assertions that say what they mean: HasCount over AreEqual on a count, IsEmpty over AreEqual against zero, AreSequenceEqual over CollectionAssert, and Contains over IsTrue around a predicate. All four are already used elsewhere in this suite, so these match the surrounding code rather than introducing a second style. MSTEST0046 is left as it was, and that is the one deliberate exception. It prefers Assert.Contains over StringAssert.Contains, but this suite calls StringAssert.Contains in 188 places and has no bare Assert.Contains anywhere. Two call sites written the other way would read as a mistake rather than as an improvement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf
|
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
2 participants
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 #57, which merged before these landed. SonarCloud's quality gate passed there, so nothing blocked the merge — but its comment reported 24 new issues without naming them, and all 24 are in code that is now on main.
Its dashboard is reachable from the agent sandbox, so these came from the API rather than from guessing.
The one that is a real defect
S2699, reported as a blocker, was right.
WhatThePaletteCreates_IsReadyToConnectreads every slot of every new node and asserts nothing about what comes back — the body is a bare call with a comment saying it must not throw.It is worse than an empty assertion, too: it has no guard against its own filter matching nothing, so it would pass just as happily if the palette stopped offering these nodes entirely. That is precisely the regression it exists to catch.
It now asserts that all three templates are found, and that every operand already sitting in a slot is the placeholder the rest of the library understands rather than a null it does not.
The rest, and where I declined one
S1192 (7) — constants where a literal repeats.
AstSchemaalready keeps one for the arguments slot's name, so the new slot names follow it; the palette's category names and the serializer'sexpectedTypekey follow the conventions their own files already set (ValueKey,MembersKey).MSTest analyzers (14) — the assertions that say what they mean:
HasCountoverAreEqualon a count,IsEmptyoverAreEqualagainst zero,AreSequenceEqualoverCollectionAssert, andContainsoverIsTruewrapped around a predicate. I checked each is already established in this suite before adopting it — 22, 4 and 10 existing call sites respectively — so these match the surrounding code rather than introducing a second style.MSTEST0046 (2) — declined, deliberately. It prefers
Assert.ContainsoverStringAssert.Contains. This suite callsStringAssert.Containsin 188 places, including tests merged from #56 the same day, and has no bareAssert.Containsanywhere in it. Two call sites written the other way would read as a mistake rather than as an improvement. Worth changing repo-wide or not at all, and that is a separate decision from this PR.Verification
dotnet buildanddotnet build -c Release— clean, 0 warnings, both target frameworks🤖 Generated with Claude Code
https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf
Generated by Claude Code