fix(casing): stop transforming an index after its part is removed - #1113
fix(casing): stop transforming an index after its part is removed#1113youdie006 wants to merge 1 commit into
Conversation
Join decrements the outer index when a transform empties a part, but the inner transform loop kept running and then indexed the shifted slice one place too low. With a leading part removed this indexes -1 and panics; elsewhere it applies a later transform twice.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The fix is minimal, directly addresses the documented root cause, and is backed by a focused regression test covering both the panic and incorrect-output scenarios.
Pull request overview
This PR fixes a panic and incorrect output in casing.Join/casing.Camel when a transform removes a part while multiple transforms are applied, by preventing subsequent transforms from running on a shifted index.
Changes:
- Stop applying remaining transforms to an index after a part is removed (break out of the inner transform loop).
- Add a regression test covering both the panic case (removed first part) and the “double-transform” incorrect output case (removed middle part).
File summaries
| File | Description |
|---|---|
| casing/casing.go | Breaks out of the inner transform loop after removing a part to avoid transforming the wrong shifted element (and negative index). |
| casing/casing_test.go | Adds a test that exercises part removal with multiple transforms to prevent panic and double-application regressions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1113 +/- ##
=======================================
Coverage 93.20% 93.20%
=======================================
Files 23 23
Lines 4988 4989 +1
=======================================
+ Hits 4649 4650 +1
Misses 272 272
Partials 67 67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
casing.Camelpanics when a transform removes the first part.Same root cause, silently wrong value when the removed part is not first:
Cause
casing/casing.go:212:i--is the outer loop's correction, but it is applied while the inner transform loop isstill running. The next transform in the list then reads
parts[i]at the decremented index — theelement before the one that was removed, which has already been transformed. With the removed part
first,
ibecomes-1.Camel(casing/casing.go:295) always doesappend(transform, strings.Title), so everyCamel/LowerCamelcall has at least two transforms and a single removing transform is enough toreach it.
The change
breakout of the inner loop after removing the part, so the outeri--andi++land on theelement that shifted into this position and it starts its transform chain from the beginning.
Why the tests missed it
TestRemovePart(casing/casing_test.go:82) exercises removal with exactly one transform, viaSnake. It still gives"one_two", unchanged by this PR. The next step — a second transform, orCamel, which supplies one implicitly — is where it breaks.Tests
TestRemovePartWithMultipleTransforms, covering both the panic and the double-transform value.Mutation-checked in both directions:
break):panic: runtime error: index out of range [-1]break, dropi--):expected: "OneTwo" actual: "oneTwo"andexpected: "onex_twox"— so the test pins the boundary from both sides rather than just one.go test -race ./...passes across all 21 packages.gofmt -l casing/empty,go vet ./casing/clean.
Observable change: only on inputs that currently panic or double-transform. No existing test
changes result.
One more, not bundled
LowerCamel("")panics too, atcasing/casing.go:300, which indexesrunes[0]without a lengthcheck — also reachable from any all-punctuation input, since
Split("_")returns an empty slice. Aseparate root cause, so I left it out to keep this PR to one; happy to fold it in or send it after.
Disclosure: found and prepared with AI assistance (Claude). Every figure above is from a run on this
branch, and the root cause is one I traced and verified by hand.