Skip to content

feat: improve array-length calling scope type incorporation#66

Merged
hughgrigg merged 1 commit into
mainfrom
feat/hg/assertArrayLength-scope-type-incorporation
Jul 8, 2026
Merged

feat: improve array-length calling scope type incorporation#66
hughgrigg merged 1 commit into
mainfrom
feat/hg/assertArrayLength-scope-type-incorporation

Conversation

@hughgrigg

@hughgrigg hughgrigg commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of readonly arrays in length assertions and matchers, with better type narrowing for nested array items.
    • Added support for preserving readonly tuple typing in more array-length checks.
  • Tests

    • Added coverage for nested readonly array scenarios to verify both compile-time typing and runtime behavior.
  • Chores

    • Updated several development tool and testing package versions.

@hughgrigg hughgrigg self-assigned this Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR rewrites the internal IsKnownReadonlyArray type predicate to use assignability checks instead of Extract-based equality, adds a new exported ReadableArrayOfLength<T, N> type, adds tests validating readonly tuple narrowing, and bumps several devDependency versions.

Changes

Readonly Array-Length Type Narrowing

Layer / File(s) Summary
Type predicate rewrite and new type export
src/assert/array-length/array-length.type.ts
IsKnownReadonlyArray now checks assignability against readonly unknown[]/unknown[] instead of Extract equality; adds exported ReadableArrayOfLength<T, N> alias.
Readonly narrowing tests
src/assert/array-length/array-length.test.ts
Adds test cases for assertArrayLength and arrayOfLength verifying readonly tuple typing and per-index property narrowing on nested readonly arrays.
Tooling version bumps
package.json
Updates devDependency version pins for Vitest, ESLint, eslint-plugin-jsdoc, Prettier, and typescript-eslint.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • KensioSoftware/smartass#43: Both PRs change type-level behavior of src/assert/array-length assertions, adjusting readonly-array matching logic and adding readonly-narrowing tests.
  • KensioSoftware/smartass#52: Both PRs modify the array-length readonly type-narrowing logic and add readonly-focused tests.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is related to the array-length type changes, but the phrasing is vague and hard to understand at a glance. Rename it to clearly describe the main change, e.g. "feat: preserve readonly tuple types in array-length assertions".
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/hg/assertArrayLength-scope-type-incorporation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/assert/array-length/array-length.type.ts (1)

47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

ReadableArrayOfLength and ReadonlyArrayOfLength are identical — DRY violation.

Both types resolve to Readonly<ArrayOfLength<T, N>> with no difference. If ReadableArrayOfLength is the new preferred name and ReadonlyArrayOfLength is 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 value

Consider extracting shared test setup.

Both test cases (lines 99-130 and 245-278) define identical ReadonlyItem/ReadonlyContainer interfaces and the same container fixture. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8beff74 and a5ceca4.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (3)
  • package.json
  • src/assert/array-length/array-length.test.ts
  • src/assert/array-length/array-length.type.ts

@hughgrigg hughgrigg merged commit 88acb54 into main Jul 8, 2026
4 checks passed
@hughgrigg hughgrigg deleted the feat/hg/assertArrayLength-scope-type-incorporation branch July 8, 2026 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant