Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin): [prefer-at] create rule #6411
feat(eslint-plugin): [prefer-at] create rule #6411
Changes from 14 commits
125a83f
acc833e
847440c
8389c82
3fd0f67
10c8592
1722700
9a2cae2
ac0bbcf
2a181be
c56944b
cc36475
a28da89
1d55c66
8a5451d
d0d0141
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 56 in packages/eslint-plugin/src/rules/prefer-at.ts
Codecov / codecov/patch
packages/eslint-plugin/src/rules/prefer-at.ts#L56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Similar lint rules to this one use the
getStaticValue
fromeslint-utils
. Have you tried using it in this file? I'm wondering if it'll help do some of the logical checks that are implemented manually right now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give me an example where this utility will help me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump for this unresolved thread @sviat9440 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump: Can you give me an example where this utility will help me??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behaviorally, it should help catch cases like this:
str[str['length'] - 1]
const key = 'length'; str[str[key] - 1]
For code, it would eliminate the need to manually write a
function getName
. Which is something a lot of our rules end up wanting. So we'd really rather than write explicit "what is the string name of this property?" logic repeatedly without a real need.https://eslint-community.github.io/eslint-utils/api/ast-utils.html#getstaticvalue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also, for anybody seeing just this thread and wondering why we're bumping at each other 😂 the comments were pending)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming] I'm finding hard to read the code with the naming convention of
isExpected*
. It's a little vague - what does 'expected' mean? Until you fully understand the code's intent that can be hard to determine it.What do you think about renaming them to more explicitly say what they do? E.g. instead of
isExpectedExpression
, something likeisArrayLengthBasedLookup
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Bug] Just remembered,
.at
's signature returnsT | undefined
. That's a change from the original type ofT
from[]
lookups. We generally put anything that can break the build into suggestions fixers rather than direct fix fixers. So this should be a suggestion fixer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Similar lint rules such as
prefer-regexp-exec
andprefer-string-starts-ends-with
use a util calledgetWrappingFixer
. Have you considered using it here? There are some edge cases in their tests I think we'd want for this one.