fix(docs): give the coverage sorts an explicit comparator - #25
Conversation
SonarCloud dropped the PR to a D reliability rating: `summarizeCatalog` sorted project names with a bare `.sort()`, which orders by string conversion — right for ASCII keys, but right by accident. Name the order instead, and not through `localeCompare`: the output is asserted in tests and read by tooling, so it must not vary with the machine's locale. Also splits `parseArgs` per mode, which `--list` had pushed past the cognitive-complexity limit, and unnests the comparator ternary.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the argument parsing and catalog sorting logic in both the TypeScript and JavaScript documentation scripts (scripts/docs.ts, scripts/lib/docs-cache.ts, and skills/spring-docs/scripts/docs.mjs). Specifically, the parseArgs function is split into cleaner helper functions (parseList and parseResolve), and a custom compareCodePoints helper is introduced to replace default .sort() and inline string comparisons, ensuring consistent, locale-independent sorting of project keys and versions. No review comments were provided, and the changes are clean, well-structured, and follow the style guidelines, so there is no additional feedback to address.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile: `compareCodePoints` overstated what it does. JavaScript compares strings by UTF-16 code unit, so a non-BMP character sorts by its leading surrogate rather than by its scalar value, and catalog keys are not constrained to ASCII by `isCatalog`. Renamed to `compareCodeUnits` and said so in the doc, rather than implementing code-point comparison: the order only has to be reproducible, code-unit order is exactly that, and a key that could expose the difference carries a character `isSafeSegment` rejects, so it never resolves to documentation whatever position it sorts into.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors argument parsing in the documentation scripts by extracting helper functions, and standardizes string sorting across TypeScript and JavaScript implementations using a new compareCodeUnits helper. A medium-severity issue was identified in the version comparison logic (compareVersions), where the comparison terminates early on numerically equivalent chunks with different representations (such as '02' and '2'), which can cause distinct versions to be incorrectly treated as identical. It is recommended to continue comparing subsequent chunks and fall back to a full string comparison to ensure correctness.
Gemini Code Assist: `compareVersions` returned the numeric difference of the first differing chunk, so a tie between differently-spelled chunks settled the whole comparison. `1.02.3` and `1.2.4` agree at `02`/`2`, returned 0, and never reached the chunk that separates them — sorting ["1.2.4", "1.02.3", "1.2.1"] gave back the input order. The loop now continues on a zero numeric diff and a run of ties falls through to `compareCodeUnits(a, b)`, so the comparator answers 0 only for strings that are actually equal. That is the property the comparator was added for, and it was not holding. Applied by hand rather than from the suggestion block: the block closed one fewer brace than its anchor range, which would have dropped the function's closing brace.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
/gemini review |
SonarCloud failed the gate on duplication, not on a rule: every one of the 18 new lines in docs-cache.test.ts repeated the fixture of the ordering test beside it, putting new-code duplication at 22.8% against a 3% limit. The two ordering tests now state a case as the list of versions and assert the order it comes back in, which is what each was actually about — the hand-built catalog literal was never the point.
There was a problem hiding this comment.
Code Review
This pull request refactors argument parsing in docs.ts and docs.mjs by extracting parseList and parseResolve helper functions. It also improves version and project name sorting in docs-cache.ts and docs.mjs by introducing a deterministic compareCodeUnits helper and updating compareVersions to properly handle numeric ties (such as '1.02.3' and '1.2.4'). A corresponding test case has been added to verify this behavior. No review comments were provided, so there is no additional feedback to address.
|
1 participant
Summary
Cherry-picks the SonarCloud reliability fix for the
--listcoverage report onto a fresh branch offmain.PR #21 was merged at its first commit while its SonarCloud gate was red. The fix commit was pushed to that PR's branch afterwards, but a merged PR's head is frozen, so the fix never reached
main. This PR carries it over. No issue tracks it.What was wrong
SonarCloud rated #21's new code D on reliability.
summarizeCatalogsorted project names with a bareObject.keys(...).sort(), which orders by string conversion (typescript:S2871). That produces the correct result for ASCII project keys, but correct by accident rather than by statement.Changes
compareCodePointscomparator that spells out the order that was previously implicit. Deliberately notlocaleCompare, which Sonar's message suggests: this output is asserted in tests and read by tooling, so a locale-sensitive collation would vary with the machine running the CLI.parseArgsintoparseList/parseResolve. Adding--listhad pushed it to cognitive complexity 17 against a limit of 15 (typescript:S3776).typescript:S3358).skills/spring-docs/scripts/docs.mjsviabun run build:skill, as the committed bundle requires.Verification
All run on this branch:
tsc --noEmit— cleaneslint --max-warnings 0— cleanbun run build:skill:check— cleandocs.test.tsanddocs-cache.test.tsNote for reviewers
This does not turn the whole gate green.
docs-cache.ts:71carries the same.sort()pattern inlookupTagand predates this work. 362 of the 418 findings open onmainsit inside the generated bundles that.sonarcloud.propertiesclaims to exclude — tracked separately as #23.Summary by cubic
Fixes the SonarCloud reliability findings on the
--listcoverage report. Project names and versions now sort with explicit comparators (compareCodeUnitsand an updatedcompareVersions) instead of relying on a bare.sort()'s implicit behavior, and the generateddocs.mjsbundle is regenerated to match.localeCompare: the output is asserted in tests and read by tooling, so the order must not vary by machine locale.isSafeSegmentrejects.compareVersionsnow continues past a numeric tie (e.g.1.02.3vs1.2.4) so a leading zero can't hide a later difference.parseArgsintoparseListandparseResolveto get--listparsing back under the cognitive-complexity limit.Written for commit eff7c17. Summary will update on new commits.