-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): Add unified-signature rule #178
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): Add unified-signature rule #178
Conversation
…m/armanio123/typescript-eslint into AddTypeScriptUnifiedSignatureRule
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.
i'm unsure if this implementation is best what we can do in here
its weird that we have to iterate over nodes few times
this is just idea and i didn't tested it but it should work:
const scopes = [];
let currentScope = {
overloads: new Map(),
parent: undefined,
typeParameters: undefined
};
function createScope(parent, typeParameters = undefined) {
scopes.push(currentScope);
currentScope = {
overloads: new Map(),
parent,
typeParameters
};
}
function checkScope() {
const failures = checkOverloads(
Array.from(currentScope.overloads.values()),
currentScope.typeParameters
);
addFailures(failures);
currentScope = scopes.pop();
}
function addOverload(signature, key = null) {
key = key || getOverloadKey(signature);
if (signature.parent === currentScope.parent && key) {
const overloads = currentScope.overloads.get(key);
if (overloads !== undefined) {
overloads.push(signature);
} else {
currentScope.overloads.set(key, [signature]);
}
}
}
return {
Program(node) {
createScope(node);
},
TSModuleBlock(node) {
createScope(node);
},
TSInterfaceDeclaration(node) {
createScope(node.body, node.typeParameters);
},
ClassDeclaration(node) {
createScope(node.body, node.typeParameters);
},
TSTypeLiteral(node) {
createScope(node);
},
// collect overloads
TSDeclareFunction(node) {
if (node.id && !node.body) {
addOverload(node, node.id.name);
}
},
TSCallSignatureDeclaration: addOverload,
TSConstructSignatureDeclaration: addOverload,
TSMethodSignature: addOverload,
TSAbstractMethodDefinition(node) {
if (!node.value.body) {
addOverload(node);
}
},
MethodDefinition(node) {
if (!node.value.body) {
addOverload(node);
}
},
// validate scopes
'Program:exit': checkScope,
'TSModuleBlock:exit': checkScope,
'TSInterfaceDeclaration:exit': checkScope,
'ClassDeclaration:exit': checkScope,
'TSTypeLiteral:exit': checkScope
};
i still don't like this code, but at-least we don't have to iterate over nodes with custom implementation
with that you are going to be able to remove: checkStatements, checkMembers, collectOverloads.
Apologies for the conflicts, we decided to remove the counts from the ROADMAP here: #225 They are too awkward to maintain manually, so this is the last time you will have to deal with syncing that up! |
@armanio123 Thanks again for your contribution, when do you think you'll be able to pick this up again? |
@@ -0,0 +1,616 @@ | |||
/** | |||
* @fileoverview Warns for any two overloads that could be unified into one by using a union or an optional/rest parameter. | |||
* @author Armando Aguirre |
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.
Sorry we've changed to using all-contributors now and removed all other occurrences of these file-level author comments, please could you remove this
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.
Removed.
…m/armanio123/typescript-eslint into AddTypeScriptUnifiedSignatureRule
Added the changes requested, migrated to TS and addressed the comments as well. Let me know if there's anything else. |
Codecov Report
@@ Coverage Diff @@
## master #178 +/- ##
=========================================
- Coverage 97.2% 97.2% -0.01%
=========================================
Files 67 68 +1
Lines 2360 2501 +141
Branches 337 385 +48
=========================================
+ Hits 2294 2431 +137
Misses 44 44
- Partials 22 26 +4
|
@@ -34,7 +34,7 @@ | |||
| [`promise-function-async`] | ✅ | [`@typescript-eslint/promise-function-async`] | | |||
| [`typedef`] | 🛑 | N/A | | |||
| [`typedef-whitespace`] | ✅ | [`@typescript-eslint/type-annotation-spacing`] | | |||
| [`unified-signatures`] | 🛑 | N/A | | |||
| [`unified-signatures`] | ✅ | [`@typescript-eslint/unified-signatures`] | |
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.
need to add the link at the bottom of the file or else this won't work
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.
Fixed.
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.
need to tighten up the types if this is going to be merged.
Most of the functions have params of type any
. Ideally there should be zero any
s.
You can import { TSESTree } from '@typescript-eslint/typescript-estree';
and use the types within that namespace to type nodes and the like.
…m/armanio123/typescript-eslint into AddTypeScriptUnifiedSignatureRule
Thanks for working through all the feedback, @armanio123! |
@uniqueiniquity is going to apply this to the TS repo this week :) |
Adding tslint's rule
unified-signature
.