Repository navigation
refactor(core): replace three comment-backed workarounds - #188
Conversation
PunGrumpy
added
pkg: core
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest Docker Doctor scan for this pull request. Learn more about Docker Doctor.
Score: 84 / 100 · 3 issues
Scanned by Docker Doctor for commit |
commit: |
loadConfig tried each candidate name in turn, with an `await` inside a `for` loop under an `eslint-disable no-await-in-loop` that gave no reason. findFirstConfigFile checks every name at once and takes the first that exists, in the same order, so the suppression goes. A new test pins the order with a .mjs, a .json and a .yaml config side by side.
volumeMount also accepted a YAML sequence as a long-syntax entry, with a comment admitting that a sequence has none of the keys it reads. It returned a mount with every field undefined, which every rule then ignored. It now returns undefined for a sequence, as for any other value that is not a mapping. A new test pins that the three mount rules stay silent on such an entry.
path.posix.normalize turns `./src` into `src`, and the trailing-slash
trim turns `./` into `.`, so `normalized.startsWith("./")` could never
match. Relative paths reach `project` through the fall-through branch,
which its comment already explains. New tests pin `.`, `./`, `./src`,
`src/data` and the two parent spellings.
34ab375 to
9680110
Compare
Description
The comment audit found three places in core where a comment exists to excuse the code next to it. This PR changes the code so the comment has nothing left to explain. Behavior does not change, and each commit adds a test that passes on the old code and on the new code.
loadConfigno longer awaits in a loop. It tried the seven config names one by one under/* eslint-disable no-await-in-loop */with no reason given.findFirstConfigFilechecks every name at once and takes the first that exists, in the same order, the wayfindRepositoryRootin the CLI already does. A new test puts a.mjs, a.jsonand a.yamlconfig side by side and checks that the.mjsone wins.volumeMountreads mappings only. It also accepted a YAML sequence, under the comment "A sequence here is invalid Compose. It has none of these keys", and returned a mount with every field undefined that all three callers then ignored. It now returnsundefinedfor a sequence, like any other value that is not a mapping. A new test checks that the three mount rules stay silent on such an entry.classifyHostPathloses an unreachable check. Afterpath.posix.normalizeand the trailing-slash trim,normalized.startsWith("./")can never match. Relative paths reachprojectthrough the last branch, whose comment already explains why. New tests pin.,./,.//,./src,src/dataand two parent spellings.Considered and left alone: the true-or-throw predicates in
schemas/config.tslook like a workaround, butanti-slop/no-runtime-typeofonly allowstypeofinside a type predicate, so the lint config forces that shape.Related Issues
N/A. Found by a comment audit of the repository.
Checklist
Screenshots (if applicable)
N/A
Additional Notes
ultracite check, typecheck and the generated docs are clean, and the PR adds no suppression.skip-changeset: no behavior visible to users changes.