Repository navigation
[eslint-miner] Add no-map-parseint-callback rule - #69608
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work — the new
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I don't see a merge-blocking issue in the changed lines from this static diff review.
Why I'm not blocking
The rule is wired into the plugin and config, and the added tests cover the primary bad patterns (`parseInt` and `Number.parseInt`) plus the obvious non-issues. I reviewed only the capped diff and did not execute code, so this is limited to static evidence from the patch.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 35.8 AIC · ⌖ 5.73 AIC · ⊞ 15.9K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd (fallback heuristic used; pr-triage was skipped as triage would add no new signal on this small, well-scoped rule-addition PR). Requesting changes for one CI-breaking omission plus a minor scope gap.
📋 Key Themes & Highlights
Key Themes
- Breaks CI:
src/index-readme-parity.test.tsasserts every rule registered inindex.tshas a matching### \rule-name`section inREADME.md. This PR registersno-map-parseint-callbackbut never touchesREADME.md`, so the parity test will fail. - Coverage gap: the rule only checks
.map(parseInt);.flatMap(parseInt)has the identical index-as-radix bug and isn't covered by a test or the implementation, even though a sibling rule in this same codebase (no-ad-hoc-jsonl-parser.ts) already treatsmap/flatMapas equivalent array callbacks. - Minor duplication: the local
isShadowedscope-walk duplicates the same logic already present inprefer-number-isnan.tsandrequire-parseInt-radix.ts— not blocking, flagged as a future extraction opportunity.
Positive Highlights
- ✅ Well-targeted rule with a clear, low-false-positive detection surface (bare unshadowed
parseInt/Number.parseIntas the first.map()argument). - ✅ Good test coverage of the shadowing edge case (
function f(parseInt) { ... }) and thethisArgform (.map(parseInt, null)). - ✅ Message is clear and actionable, explaining the exact failure mode with a concrete example.
Please update README.md to keep the parity test green, and consider extending to flatMap for full coverage of the bug class.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 48.4 AIC · ⌖ 14 AIC · ⊞ 10.1K
Comment /matt to run again
Comments that could not be inline-anchored
eslint-factory/src/index.ts:90
[/codebase-design] This rule is registered in index.ts but README.md is never updated, which will break the automated parity test (src/index-readme-parity.test.ts) that asserts every exported rule has a matching ### rule-name`` heading in the README.
<details>
<summary>💡 Fix</summary>
Add a table row and a ### \no-map-parseint-callback`section toREADME.md, following the format used for the neighbouring prefer-number-isnan` rule (flagged forms, rationale, and any intentio…
eslint-factory/src/rules/no-map-parseint-callback.ts:86
[/codebase-design] isShadowed duplicates the hasLocalBinding scope-walk already implemented in prefer-number-isnan.ts and require-parseInt-radix.ts. Minor duplication, but worth noting since this codebase already has 3+ copies of the same scope-chain lookup.
<details>
<summary>💡 Suggestion</summary>
Not blocking for this PR, but a good /improve-codebase-architecture follow-up would be extracting a shared hasLocalBinding(sourceCode, node, name) helper (e.g. in a `scope-utils.t…
eslint-factory/src/rules/no-map-parseint-callback.ts:99
[/tdd] The rule only matches callee property name "map", but .flatMap(parseInt) has the exact same index-as-radix bug and is used elsewhere in this very codebase (no-ad-hoc-jsonl-parser.ts treats map/flatMap/forEach/etc. as equivalent array callbacks). No test exercises flatMap, so this gap is currently undetected.
<details>
<summary>💡 Suggested fix</summary>
const arrayCallbacks = new Set(["map", "flatMap"]);
// ...
if (callee.type !== "MemberExpression" || callee.co…
</details>There was a problem hiding this comment.
🟡 Changes recommended
Missing rule documentation breaks the existing README/export parity test.
2 open issues
What changed in this PR
Adds a custom ESLint warning to prevent incorrect parsing when .map() passes an element index to parseInt as its radix.
Changes:
- Detects unshadowed
parseIntandNumber.parseIntcallbacks. - Adds rule tests.
- Registers and enables the rule at warning level.
| File | Description |
|---|---|
eslint-factory/src/rules/no-map-parseint-callback.ts |
Implements callback detection and diagnostics. |
eslint-factory/src/rules/no-map-parseint-callback.test.ts |
Tests matching, exclusions, and documentation URL. |
eslint-factory/src/index.ts |
Registers the rule in the plugin. |
eslint-factory/eslint.config.cjs |
Enables the warning. |
Review effort: Max
| "prefer-get-error-message": preferGetErrorMessageRule, | ||
| "prefer-get-error-message-over-string": preferGetErrorMessageOverStringRule, | ||
| "prefer-number-isnan": preferNumberIsNanRule, | ||
| "no-map-parseint-callback": noMapParseIntCallbackRule, |
| if (node.type === "Identifier") { | ||
| return node.name === "parseInt" && !isShadowed(node, "parseInt"); | ||
| } | ||
| return node.type === "MemberExpression" && !node.computed && node.object.type === "Identifier" && node.object.name === "Number" && !isShadowed(node, "Number") && node.property.type === "Identifier" && node.property.name === "parseInt"; |
|
🎉 This pull request is included in a new release. Release: |
Adds
gh-aw-custom/no-map-parseint-callback(warn), flagging.map(parseInt)/.map(Number.parseInt)where the element index becomes the radix (["10","10","10"].map(parseInt)→[10, NaN, 2]).Evidence: actions/setup/js already uses
.map(Number)for version/date parsing (e.g.check_version_updates.cjs,pi_runtime.cjs); the neighbouringparseIntradix rule does not cover this callback form. Low false-positive risk: only fires on a bare unshadowedparseIntas the first argument to.map.Validation:
npm run build,npm run lint:setup-js(0 errors, no new warnings from this rule), and the rule unit test pass.