Skip to content
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

Repo: Add a util for determining whether a node is a specific identifier #8327

Closed
StyleShit opened this issue Jan 31, 2024 · 4 comments
Closed
Labels
locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. repo maintenance things to do with maintenance of the repo, and not with code/docs wontfix This will not be worked on

Comments

@StyleShit
Copy link
Contributor

Suggestion

Splits from #8295 (comment)

The proposal is to go over all of the places where we check if a node is a specific identifier and use a util for that. Here are a couple of examples of places we do this:

if (
node.typeName.type === AST_NODE_TYPES.Identifier &&
node.typeName.name === 'Array'
) {

return (
node.typeName.type === AST_NODE_TYPES.Identifier &&
node.typeName.name === 'const'
);

return (
node.type === AST_NODE_TYPES.TSTypeReference &&
node.typeName.type === AST_NODE_TYPES.Identifier &&
['Array', 'ReadonlyArray'].includes(node.typeName.name)
);

I think we can take inspiration from this existing function and extract it to a util:

function isIdentifier(
init: TSESTree.Expression,
...names: string[]
): boolean {
return (
init.type === AST_NODE_TYPES.Identifier && names.includes(init.name)
);
}

It might be a nice good first issue PR 😄

@StyleShit StyleShit added repo maintenance things to do with maintenance of the repo, and not with code/docs triage Waiting for maintainers to take a look labels Jan 31, 2024
@bradzacher
Copy link
Member

bradzacher commented Feb 4, 2024

Personally - I don't think there's much value in extracting two logical expressions into a utility function.

Eg

if (
  node.typeName.type === AST_NODE_TYPES.Identifier &&
  node.typeName.name === 'Array'
) {...}

// vs

if (
  isIdentifierWithName(
    node.typeName,
    'Array',
  )
) {...}

I don't think you gain anything in terms of conciseness or readability, and I think such an abstraction costs you more here as there's a function call involved.

So I'm personally a -1 for this.

Cc @typescript-eslint/triage-team

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Feb 4, 2024
@Josh-Cena
Copy link
Member

I'm -0.5, but I think isIdentifierWithName(node.typeName, ['Array', 'ReadonlyArray']) does look slightly more semantic than its expanded form. Still have no appetite for small utilities because most contributors aren't going to be aware of them and just write it again anyway.

@StyleShit
Copy link
Contributor Author

I think it's nicer to read, but @Josh-Cena has a good point here. As a (fairly?) new contributor, I'm not aware of some utils. I always try to find the right ones (which is kinda guessing my way between all those files & functions), but it's pretty difficult.
So now I'm not sure that this change will be useful other than having a (subjectively) nicer code in some places...

@Josh-Cena
Copy link
Member

There doesn't seem to be enough interest in this.

@Josh-Cena Josh-Cena closed this as not planned Won't fix, can't repro, duplicate, stale Jun 1, 2024
@Josh-Cena Josh-Cena added wontfix This will not be worked on and removed awaiting response Issues waiting for a reply from the OP or another party labels Jun 1, 2024
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Jun 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. repo maintenance things to do with maintenance of the repo, and not with code/docs wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants