Improve no identical title #375
Conversation
SimenB
force-pushed
the
improve-no-identical-title
branch
from
768d381 to
93187c3
Compare
src/rules/tsUtils.ts
Outdated
| * | ||
| * i.e `KnownCallExpression<'includes'>` represents `.includes()`. | ||
| */ | ||
| export interface KnownCallExpression<Name extends string = string> |
There was a problem hiding this comment.
no need to export, but I guess we can clean that up later?
There was a problem hiding this comment.
Yeah, exactly - it does need to be exported later for at least one rule.
I don't think it's worth worrying about exports too much right now.
Same with possible use of varargs, as you're about to see in my next PR for no-large-snapshots :P
There was a problem hiding this comment.
They're circular: KnownCallExpression is used by CalledKnownMemberExpression, which is used by KnownCallExpression, and extends KnownMemberExpression.
You're right: they're not needed in this PR.
There was a problem hiding this comment.
KnownCallExpression is only used in CalledKnownMemberExpression, which is only used in KnownCallExpression. It seems we only need getAccessorValue here? Test coverage won't complain about unused types as they're stripped out (is there some tsc option we can add that yells about it?)
There was a problem hiding this comment.
hah, right. GH didn't feel like telling me you had commented...
There was a problem hiding this comment.
Test coverage won't complain about unused types as they're stripped out (is there some tsc option we can add that yells about it?)
Yeah I've been wondering that myself - as far as I know tsc doesn't have such an option, but I think it'd be a pretty big can of worms.
I'm happy to investigate after we've got the bulk of things merged- IntelliJ has an inspection that'll pick them up (which they've now exposed via API, so technically you could use that in CI.... 😜)
I've just checked @typescript-eslint, and they don't have a rule for it.
Right now I think it'd be an annoying can of worms - I'm already running into trouble w/ the fact that an AccessorNode can be an Identifier, but no rule calls getAccessorValue on an Identifier (it'll make sense when you see the PR in a sec).
There was a problem hiding this comment.
ugh yeah it seems to be a slow day in the tech world - I'm trying to push a new commit that removes those interfaces, but it's being sllooooww (might have something to do w/ having the jest monorepo open in another project, but eh 🤷♂)
|
@SimenB Can I merge, since you've approved this? |
|
Mind resolving the conflict? Then feel free to merge |
|
Sweet as. I'm about to make a couple of similar PRs that'll likely conflict, and then head to bed: so if you approve them, I'm happy to resolve conflicts & merge them when I'm up tomorrow :) But first, got a PR to review: someone over on jest wants someone who knows TS to eye up his PR 😉 |
G-Rath
force-pushed
the
improve-no-identical-title
branch
from
12e90dd to
cacc150
Compare
|
🎉 This PR is included in version 22.15.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is based off #374, which should be reviewed & merged first.
I had to simplify
getAccessorValue&AccessorNodeto not handleIdentifiernodes, as nothing covers that yet.Otherwise it's a direct port.