Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

[FEAT] Add 'callable-types' rule #280

Closed

Conversation

uniqueiniquity
Copy link

@uniqueiniquity uniqueiniquity commented Jan 8, 2019

  • I’ve filled out the documentation, including all relevant sections and a link to the equivalent TSLint rule if available.
  • I’ve filled out the rule’s category
  • I’ve added documentation for the equivalent TSLint rule (in extraDescription)
  • I’ve added the rule documentation link via the helper from ../utils.
  • I’ve added the rule to the roadmap
  • I’ve added tests for my changes

Adds callable-types rule from TSLint. Depends on eslint/typescript-eslint-parser#568 for node mapping.

function getESTreeType(syntaxKind) {
const tsSyntaxKindName = ts.SyntaxKind[syntaxKind];

return parser.Syntax[tsSyntaxKindName] || `TS${tsSyntaxKindName}`;
Copy link
Contributor

@armano2 armano2 Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not always a case, sometimes names do change, i don't think it's good idea..

i think its better to import types from

https://github.com/JamesHenry/typescript-estree/blob/master/src/ast-node-types.ts
"typescript-estree/dist/ast-node-types.js"

with that we don't have to convert names back and forward...

number (SyntaxKind) -> string (SyntaxKind) -> string (not always correct ts node)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I'm doing; parser.Syntax is pulling from there, it just seemed right to do it through typescript-eslint-parser as opposed to directly referencing typescript-estree. The second case is just backup for nodes missing from the file you linked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are also nodes not present in ts, like TSTypeParameterDeclaration, TSTypeParameterInstantiation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hmm, I didn't see your edit. Let me see if that works better.

Copy link
Contributor

@armano2 armano2 Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also in some rare cases this using conversion ts.SyntaxKind[syntaxKind] will return you invalid name

this enum contains also FirstAssignment, LastAssignment and so on, it will convert it to last key from enum with provided value

for example:
instead of TemplateTail you will get LastTemplateToken

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TemplateTail is not present in estree its called TemplateElement :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I totally see your point. I'll put together a different approach later today.

@@ -126,7 +126,7 @@
| [`arrow-parens`] | 🌟 | [`arrow-parens`](https://eslint.org/docs/rules/arrow-parens) |
| [`arrow-return-shorthand`] | 🌟 | [`arrow-body-style`](https://eslint.org/docs/rules/arrow-body-style) |
| [`binary-expression-operand-order`] | 🌟 | [`yoda`](https://eslint.org/docs/rules/yoda) |
| [`callable-types`] | 🛑 | N/A |
| [`callable-types`] | | [`typescript/callable-types`] |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t forget to add this reference to the section at the bottom of the page so the link works properly.

docs/rules/callable-types.md Outdated Show resolved Hide resolved
Co-Authored-By: uniqueiniquity <uniqueiniquity@users.noreply.github.com>
@bradzacher bradzacher added the WIP PRs that are work in progress label Jan 9, 2019
@bradzacher
Copy link
Owner

One thing I'm curious about here - how come this rule requires the parser services?
It seems like this would be achievable using just the AST from the parser?

@armano2
Copy link
Contributor

armano2 commented Jan 12, 2019

@bradzacher you are correct, but there has to be some changes done before we can merge it :)

extraDescription: [util.tslintRule("callable-types")],
url: util.metaDocsUrl("callable-types"),
},
fixable: "code", // or "code" or "whitespace"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this comment?

Suggested change
fixable: "code", // or "code" or "whitespace"
fixable: "code",

docs: {
description:
"Use function types instead of interfaces with call signatures",
category: "Style",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be TypeScript instead?

@bradzacher bradzacher closed this Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
WIP PRs that are work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants