Conversation
gundermanc
force-pushed
the
gundermanc/disallow-underspecified-types
branch
from
d357c26 to
c2f47af
Compare
|
Size Change: +1.46 kB (+0.01%) Total Size: 26 MB
ℹ️ View Unchanged
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the codebase's type safety and maintainability by introducing a new ESLint rule. The rule targets a common pattern of checking object property types using Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ESLint rule. My review focuses on a maintainability improvement in the ESLint configuration file itself, where I suggest using a shared constant to avoid duplicating rule definitions.
Note: Security Review is unavailable for this PR.
eslint.config.js
Outdated
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| { | ||
| selector: 'CallExpression[callee.name="require"]', | ||
| message: 'Avoid using require(). Use ES6 imports instead.', | ||
| }, | ||
| { | ||
| selector: 'ThrowStatement > Literal:not([value=/^\\w+Error:/])', | ||
| message: | ||
| 'Do not throw string literals or non-Error objects. Throw new Error("...") instead.', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| { | ||
| files: [ | ||
| 'integration-tests/**/*.ts', | ||
| 'evals/**/*.ts', | ||
| ], | ||
| rules: { | ||
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| { | ||
| selector: 'CallExpression[callee.name="require"]', | ||
| message: 'Avoid using require(). Use ES6 imports instead.', | ||
| }, | ||
| { | ||
| selector: 'ThrowStatement > Literal:not([value=/^\\w+Error:/])', | ||
| message: | ||
| 'Do not throw string literals or non-Error objects. Throw new Error("...") instead.', | ||
| }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
To improve maintainability and avoid code duplication, please use the commonRestrictedSyntaxRules constant that is already defined in this file. Both of these new no-restricted-syntax blocks are duplicating the rules from that constant.
'no-restricted-syntax': ['error', ...commonRestrictedSyntaxRules],
},
},
{
files: [
'integration-tests/**/*.ts',
'evals/**/*.ts',
],
rules: {
'no-restricted-syntax': ['error', ...commonRestrictedSyntaxRules],
},
},
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new ESLint rule to disallow checking object property types with typeof obj['property'], guiding developers towards using safer type guard functions. This is a great step for improving type safety and maintainability. The strategy of introducing the rule and disabling it for existing violations is a practical approach for a large codebase. I've found one minor area for improvement in the ESLint configuration to reduce code duplication.
Note: Security Review did not run due to the size of the PR.
eslint.config.js
Outdated
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| { | ||
| selector: 'CallExpression[callee.name="require"]', | ||
| message: 'Avoid using require(). Use ES6 imports instead.', | ||
| }, | ||
| { | ||
| selector: 'ThrowStatement > Literal:not([value=/^\\w+Error:/])', | ||
| message: | ||
| 'Do not throw string literals or non-Error objects. Throw new Error("...") instead.', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
To improve maintainability and prevent the configurations from drifting apart, you can reuse the commonRestrictedSyntaxRules constant here instead of duplicating the rules. The same applies to the configuration for integration-tests and evals below.
'no-restricted-syntax': ['error', ...commonRestrictedSyntaxRules],
…nderspecified-types
3 participants
Summary
Adds a new eslint rule that disallows syntax like the following outside of tests.
Why
This syntax is almost exclusively used in the codebase as a form of inline type assertion for unpacking properties on an object of
unknownorRecord<string, unknown>type.I believe that
typeof expression == 'string'is a reasonable type assertion, but using this same pattern to check object type params is a recipe for bugs.isMyObject().The previously verbose type guards...
...can be rewritten as...
Alternatives
Instead of the above, I recommend the following:
When should I use or not use Record<string, unknown>
You can still use Record<string, unknown>, when you know you are dealing with a string keyed unknown object type where you need to dynamically test the value type due to a lack of consistent schema. In general though you should pass objects of unknown type around as
unknownand use type guard functions to check the type, as needed. If you know at compile time that an object can be only one of a few different types, or have members of a specific set of types, use discriminated unions and generics to represent these possibilities.Fixes ##21499