Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
…m/armanio123/typescript-eslint into AddTypeScriptUnifiedSignatureRule
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
armano2 left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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:
constscopes=[];letcurrentScope={overloads:newMap(),parent:undefined,typeParameters:undefined};functioncreateScope(parent,typeParameters=undefined){scopes.push(currentScope);currentScope={overloads:newMap(), parent, typeParameters};}functioncheckScope(){constfailures=checkOverloads(Array.from(currentScope.overloads.values()),currentScope.typeParameters);addFailures(failures);currentScope=scopes.pop();}functionaddOverload(signature,key=null){key=key||getOverloadKey(signature);if(signature.parent===currentScope.parent&&key){constoverloads=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 overloadsTSDeclareFunction(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.
Uh oh!
There was an error while loading.Please reload this page.
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. |
codecovbot commentedMar 11, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 typeany
. Ideally there should be zeroany
s.
You canimport { 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
.