feat: improve array-length calling scope type incorporation#66
Conversation
📝 WalkthroughWalkthroughThis PR rewrites the internal ChangesReadonly Array-Length Type Narrowing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/assert/array-length/array-length.type.ts (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ReadableArrayOfLengthandReadonlyArrayOfLengthare identical — DRY violation.Both types resolve to
Readonly<ArrayOfLength<T, N>>with no difference. IfReadableArrayOfLengthis the new preferred name andReadonlyArrayOfLengthis kept for backward compatibility, make the relationship explicit by aliasing one to the other:♻️ Suggested refactor
export type ReadableArrayOfLength<T, N extends number> = Readonly< ArrayOfLength<T, N> >; -export type ReadonlyArrayOfLength<T, N extends number> = Readonly< - ArrayOfLength<T, N> ->; +export type ReadonlyArrayOfLength<T, N extends number> = + ReadableArrayOfLength<T, N>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/assert/array-length/array-length.type.ts` around lines 47 - 54, The two type aliases in array-length.type.ts are duplicated, so make the relationship explicit by keeping ReadableArrayOfLength as the preferred name and turning ReadonlyArrayOfLength into a direct alias of it (or vice versa) instead of repeating the same Readonly<ArrayOfLength<T, N>> definition. Update the array-length type declarations so the intent is clear and future changes only need to be made in one place.src/assert/array-length/array-length.test.ts (1)
245-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared test setup.
Both test cases (lines 99-130 and 245-278) define identical
ReadonlyItem/ReadonlyContainerinterfaces and the samecontainerfixture. Extracting these to module-scoped declarations would reduce duplication and keep the tests focused on the behavior under test.This is optional since test-file duplication is often acceptable for readability.
♻️ Optional refactor: shared fixture
+ interface ReadonlyItem { + readonly id: string; + readonly enabled: boolean; + } + + interface ReadonlyContainer { + readonly items: readonly ReadonlyItem[]; + } + + const readonlyContainer: ReadonlyContainer = { + items: [ + { id: "first", enabled: true }, + { id: "second", enabled: false }, + ], + }; + // ... in test 1: - interface ReadonlyItem { - readonly id: string; - readonly enabled: boolean; - } - - interface ReadonlyContainer { - readonly items: readonly ReadonlyItem[]; - } - - const container: ReadonlyContainer = { - items: [ - { id: "first", enabled: true }, - { id: "second", enabled: false }, - ], - }; + const container = readonlyContainer;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/assert/array-length/array-length.test.ts` around lines 245 - 278, The two test cases in the array-length test file duplicate the same ReadonlyItem and ReadonlyContainer definitions plus the same container fixture. Extract those shared declarations to module scope and reuse them in both the existing nested readonly array test and the earlier test, keeping the assertions in each test focused on the behavior under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/assert/array-length/array-length.test.ts`:
- Around line 245-278: The two test cases in the array-length test file
duplicate the same ReadonlyItem and ReadonlyContainer definitions plus the same
container fixture. Extract those shared declarations to module scope and reuse
them in both the existing nested readonly array test and the earlier test,
keeping the assertions in each test focused on the behavior under test.
In `@src/assert/array-length/array-length.type.ts`:
- Around line 47-54: The two type aliases in array-length.type.ts are
duplicated, so make the relationship explicit by keeping ReadableArrayOfLength
as the preferred name and turning ReadonlyArrayOfLength into a direct alias of
it (or vice versa) instead of repeating the same Readonly<ArrayOfLength<T, N>>
definition. Update the array-length type declarations so the intent is clear and
future changes only need to be made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff1bab1a-24fd-403c-91c6-5afbfea5d0dd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonsrc/assert/array-length/array-length.test.tssrc/assert/array-length/array-length.type.ts
Summary by CodeRabbit
Bug Fixes
Tests
Chores