Adopt ktsu.Semantics for paths and colours - #69
Merged
Conversation
Replaces raw path strings and System.Drawing colour handling with the ktsu.Semantics types, per the ktsu convention of preferring semantic types for strings and paths. Paths: Arguments.TryResolveInput and TryResolveOutput resolve the raw option strings into AbsoluteDirectoryPath, normalising relative values against the working directory first because the semantic type only accepts absolute paths. Validate calls both, which turns two crashes into clean exit 1 errors: - an --input directory that does not exist previously threw an unhandled DirectoryNotFoundException from Directory.GetFiles - an --input that is a file rather than a directory previously threw an unhandled IOException Output file paths are now composed with the / operator over FileName rather than Path.Join, which also rejects a name carrying a directory separator. Colours: ColorParser.TryParse accepts a NamedColors name or a hex value, and System.Drawing is gone. This adds #RGB shorthand and #RRGGBBAA alpha support, and an unparseable colour is now caught by validation instead of throwing part way through a run. BREAKING: the named colour set narrows from the roughly 140 CSS names that System.Drawing.ColorTranslator understood to the 13 in ktsu.Semantics NamedColors. Any other colour must now be given as hex. Pinned by ColorParserTests.RejectsCssNamesOutsideTheKnownSet. Semantics.Color stores linear channels as doubles, so ProcessImage encodes to sRGB bytes once up front rather than per pixel. FromHex().ToBytes() round trips byte for byte, verified across all 256 greyscale values before the swap, which is why every gold master image is unchanged. Supporting changes: Polyfill moves from a 9.7.1 VersionOverride to 11.0.1 centrally, which is the floor ktsu.Semantics.Strings requires, and that package is referenced directly because SemanticString.Create lives there and the ktsu analyzer rejects transitive use. Adds 17 tests covering colour parsing and the new path validation rules. Claude-Session: https://claude.ai/code/session_01Ruh9nAQiuU5VGHV8uJCXtu
S3626, redundant jump. The continue at the end of the per file catch block was the last statement in the loop body, so it did nothing. The guard continue that skips .new.png files earlier in the loop is a real one and stays. S3776, cognitive complexity 25 against a limit of 15 in ProcessImage. The method carried the whole pipeline inline, including two nested pixel loops with their own branching. The passes are now named methods, FindBrightestOpaqueValue, TintAndMeasureBounds and CropSquareAndResize, leaving ProcessImage as a readable sequence of steps with a single branch for the blank image case. The inclusive bounding box is now a PixelBounds record struct rather than four loose locals threaded through the method. Its Width and Height carry the +1 that the inclusive indices require, which is the off-by-one fixed earlier, and IsEmpty names the inverted bounds check that detects an image with nothing opaque in it. Behaviour is unchanged. All 65 tests pass, including the gold master cases, which compare output pixel for pixel. Claude-Session: https://claude.ai/code/session_01Ruh9nAQiuU5VGHV8uJCXtu
SonarCloud reported 12 issues on the new test files, all MSTEST0037 and MSTEST0046 suggesting the newer assertion methods. - Assert.AreEqual(n, errors.Count) becomes Assert.HasCount(n, errors) - Assert.AreEqual(0, errors.Count) becomes Assert.IsEmpty(errors) - StringAssert.Contains becomes Assert.Contains The last one is not a rename. StringAssert.Contains takes the haystack first, Assert.Contains takes the needle first, so the arguments had to swap. Both parameters are strings, so getting that backwards would still compile. It would not pass, because the assertion would then be checking whether a long error message is a substring of a short literal, and the suite passing is what confirms the order is right. Also converted the two equivalent AreNotEqual checks in GoldMasterTests, which the analyzer raised back when the test suite landed and which were never cleared. They were not in this run's report because that file is unchanged here, so they no longer count as new code. All 65 tests still pass. Claude-Session: https://claude.ai/code/session_01Ruh9nAQiuU5VGHV8uJCXtu
|
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.
Summary
Replaces raw path strings and
System.Drawingcolour handling with the ktsu.Semantics types, per the ktsu convention of preferring semantic types for strings and paths. Also clears the two SonarCloud smells left over from #66.Warning
This narrows the accepted colour names. See the breaking change section below.
Paths
Arguments.TryResolveInputandTryResolveOutputresolve the raw option strings intoAbsoluteDirectoryPath, normalising relative values against the working directory first, because the semantic type only accepts absolute paths.Validatecalls both, which turns two crashes into clean exit 1 errors:--inputthat does not existDirectoryNotFoundException, stack trace--input directory does not exist: ..., exit 1--inputthat is a fileIOException, stack trace--input is a file, not a directory: ..., exit 1Both previously escaped the per file catch because
Directory.GetFilesruns outside it, so they crashed the process and undermined the exit codes added in #66.Output file paths are now composed with the
/operator overFileNamerather thanPath.Join, which also rejects a name carrying a directory separator.Colours
ColorParser.TryParseaccepts aNamedColorsname or a hex value, andSystem.Drawingis gone. This adds#RGBshorthand and#RRGGBBAAalpha support, and an unparseable colour is caught by validation rather than throwing part way through a run.Colorstores linear channels as doubles, soProcessImageencodes to sRGB bytes once up front rather than per pixel.Breaking change
The named colour set narrows from the roughly 140 CSS names
System.Drawing.ColorTranslatorunderstood to the 13 inNamedColors:black,white,red,green,blue,yellow,cyan,magenta,gray,grey,orange,purple,transparent.Anything else must now be given as hex.
CornflowerBluewas previously documented in the README and no longer works. Pinned byColorParserTests.RejectsCssNamesOutsideTheKnownSetand the README is updated.Sonar smells from #66
continueat the end of the per file catch was the last statement in the loop body.ProcessImage. The pixel passes are now named methods,FindBrightestOpaqueValue,TintAndMeasureBoundsandCropSquareAndResize, leavingProcessImagea readable sequence with one branch.The inclusive bounding box is now a
PixelBoundsrecord struct rather than four loose locals. ItsWidthandHeightcarry the+1the inclusive indices require, which is the off-by-one fixed in #66, andIsEmptynames the inverted bounds check.Supporting changes
Polyfill moves from a
9.7.1VersionOverrideto11.0.1centrally, which is the floorktsu.Semantics.Stringsrequires, and that package is referenced directly becauseSemanticString.Createlives there and the ktsu analyzer rejects transitive use.Test plan
dotnet build -c Releaseclean, 0 warnings with warnings as errors enableddotnet test -c Releasepasses 65 of 65, up from 48. 17 new tests cover colour parsing and the path validation rulesFromHex().ToBytes()round trips byte for byte across all 256 greyscale values and matchesColorTranslator.FromHtmlon sampled values, so the colour change is provably a no-op on output. The gold masters passing unchanged confirms it, and also confirms theProcessImagerefactor is behaviour preserving#F80shorthand and theorangenamed colour🤖 Generated with Claude Code
https://claude.ai/code/session_01Ruh9nAQiuU5VGHV8uJCXtu