Apply the small-value-space guard to null-valued constants in no-duplicate-constant-values - #51542
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
|
|
🎉 Thanks for this contribution! This PR looks great and is ready for review. Summary: The fix properly extends the small-value-space guard from numeric and boolean constants to What stands out:
This is a solid, focused fix that improves the linter's accuracy. Well done!
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed the 3-file diff (README sentence, one test case, and a getMinDuplicateGroupSize extraction that replaced a nested ternary with an equally-sized named function). No dead code, speculative flexibility, or reinvented stdlib found.
|
There was a problem hiding this comment.
Pull request overview
Updates the ESLint rule to reduce false positives for repeated null constants.
Changes:
- Applies a minimum duplicate-group size of three to
null. - Adds valid and invalid
nulltest cases. - Documents the updated threshold behavior.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-duplicate-constant-values.ts |
Adds and applies the null threshold. |
eslint-factory/src/rules/no-duplicate-constant-values.test.ts |
Tests groups of two and three null constants. |
eslint-factory/README.md |
Documents null threshold behavior. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause properly addressed:
nullkeys asobject:nulland now gets the same guard as booleans and numbers - ✅ Clean extraction of the nested ternary into
getMinDuplicateGroupSize()— improves readability and extensibility - ✅
NULL_VALUE_KEYconstant avoids a magic string and makes the exact-match intent explicit - ✅ Tests mirror the boolean guard test structure, covering both the valid (2 constants) and invalid (3 constants) case
- ✅ README updated in the same commit
Minor Observation
The two-null valid case is asserted twice: in the catch-all valid block and in the dedicated null threshold test. Neither is incorrect, but the first occurrence is slightly redundant — removing it from the catch-all would keep each test focused on a single concern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 15.4 AIC · ⌖ 7.45 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
The implementation is correct and well-reasoned. null has a value space of 1, making the same minimum-group-size guard appropriate as for booleans. The refactoring into getMinDuplicateGroupSize also improves readability. Tests and docs are updated consistently.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.5 AIC · ⌖ 6.73 AIC · ⊞ 5.5K
|
🎉 This pull request is included in a new release. Release: |
no-duplicate-constant-valuesraises the minimum duplicate-group size to 3 for numbers and booleans to avoid coincidental matches in small value spaces, butnullliterals key toobject:nulland so fell through to the generic threshold of 2.nullhas exactly one possible value — a smaller space than booleans — so it is the primitive most prone to the false positive the boolean guard exists to prevent:Changes
src/rules/no-duplicate-constant-values.ts— addedMIN_NULL_DUPLICATE_GROUP_SIZE = 3(and aNULL_VALUE_KEYconstant) with a comment mirroring the existing boolean rationale. Extracted the threshold selection out of the inline nested ternary intogetMinDuplicateGroupSize(valueKey), which now covers thenullcase. Matches exactly onobject:nullrather than anobject:prefix, since regexps are keyed separately.src/rules/no-duplicate-constant-values.test.ts— valid case for twonullconstants; invalid case for three, mirroring the boolean test structure.README.md— rule description now listsnullalongside numeric and boolean in the raised-threshold sentence.String, number, boolean, and regexp behavior is unchanged.
undefinedis an identifier rather than a literal and was already out of scope.