fix: drop the backtracking trailing-whitespace regex - #144
Merged
Conversation
replaceAll("\s*$", "") has super-linear runtime on long lines. String.stripTrailing()
(Java 11+) does the same job with no regex engine involved, and handles Unicode
whitespace the ASCII-only \s class misses.
Also deletes testNormalizeCollectionWithNonComparableSet, which built a Set and
asserted on new ArrayList<>(set) without ever calling save() -- it exercised the
JDK, not YamlWriter. normalizeCollection stays at 6/6 branches covered without it.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
The test built a Set and asserted on new ArrayList<>(set) without ever calling save(), so it covered the JDK rather than normalizeCollection. It now saves a Set<Address> -- records aren't Comparable -- and asserts insertion order survives, which fails with a ClassCastException if the Comparable guard is ever dropped. Also corrects the CoverageBoostTests comment that claimed to use a List when it uses a Set of Comparable UUIDs.
|
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
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.



Clears the two Sonar findings on master that are worth code changes. 263 tests green.
Fixed — the two MAJOR regex findings
YamlFileInterface:315andYamlWriter:274both didline.replaceAll("\\s*$", "")to trim a comment line. Sonar is right that it's super-linear. The stdlib has done this since Java 11 and the project targets 17:No regex engine, clearer intent, and
stripTrailing()usesCharacter.isWhitespaceso it also catches Unicode whitespace that the ASCII-only\sclass misses.Fixed — the MINOR "unused collection", by making the test real
YamlWriterEdgeCaseTests.testNormalizeCollectionWithNonComparableSetbuilt aSet, then asserted onnew ArrayList<>(nonComparableSet)— it never calledsave(), so it covered the JDK's ArrayList constructor rather thannormalizeCollection. That's what Sonar's "unused collection" was pointing at.Rather than delete it, it now does what its name says: saves a
Set<Address>(records aren'tComparable) and asserts the insertion order survives, since a set that can't be sorted must be written as it came in.Verified it actually guards the branch — dropping the
instanceof Comparablecheck fromnormalizeCollectionmakes it fail with aClassCastExceptionrather than pass quietly.Not changed — two I'd suppress instead
TypeConverter:79, cognitive complexity 21. This is the flat dispatch table deliberately left alone in #143: fourteen sequentialif (...) return ...;guards with zero nesting, whose order is the meaning. It was WONTFIX'd before, and only reappeared because #141 shifted its declaration line and Sonar lost the issue↔suppression match. Worth re-marking WONTFIX rather than splitting.PluginLoggerRoutingTest:63, unused parameterargs.VarargsStyleLogger.warning(String, Object...)exists precisely sodiscoverPluginSinkcan find that signature by reflection — the unused parameter is the thing under test. Removing it deletes the test's reason to exist. Worth marking false-positive.Also
Corrected the
CoverageBoostTests.testNonComparableSetInYamlcomment, which said "UUIDs are Comparable, so let's use a List instead to verify the path" while using aSet<UUID>. It covers the sorting side; the comment now says so and points at the test that covers the other side.