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): new rule method-signature-style #1685
Merged
bradzacher
merged 10 commits into
typescript-eslint:master
from
phaux:method-signature-style
Apr 3, 2020
Merged
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
fd93d53
feat(eslint-plugin): method-signature-style
phaux 995a423
feat(eslint-plugin): [method-sig-style] add fixers
phaux be1067c
docs(eslint-plugin): [method-sig-style] add docs
phaux cca3d68
test(eslint-plugin): [method-sig-style] more tests
phaux f42efe0
docs(eslint-plugin): docs
phaux 130781b
Merge branch 'master' into method-signature-style
bradzacher 291f2b0
feat(eslint-plugin): preserve comments in method fixer
phaux b9f505d
Merge branch 'master' into method-signature-style
bradzacher 932ae62
Merge branch 'master' into method-signature-style
phaux f355e4e
style(eslint-plugin): add workaround for failing lint
phaux File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
83 changes: 83 additions & 0 deletions
83
packages/eslint-plugin/docs/rules/method-signature-style.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
# Enforces using a particular method signature syntax. (`method-signature-style`) | ||
|
||
There are two ways to define an object/interface function property. | ||
|
||
```ts | ||
// method shorthand syntax | ||
interface T1 { | ||
func(arg: string): number; | ||
} | ||
|
||
// regular property with function type | ||
interface T2 { | ||
func: (arg: string) => number; | ||
} | ||
``` | ||
|
||
A good practice is to use the TypeScript's `strict` option (which implies `strictFunctionTypes`) which enables correct typechecking for function properties only (method signatures get old behavior). | ||
|
||
TypeScript FAQ: | ||
|
||
> A method and a function property of the same type behave differently. | ||
> Methods are always bivariant in their argument, while function properties are contravariant in their argument under `strictFunctionTypes`. | ||
|
||
See the reasoning behind that in the [TypeScript PR for the compiler option](https://github.com/microsoft/TypeScript/pull/18654). | ||
|
||
## Options | ||
|
||
This rule accepts one string option: | ||
|
||
- `"property"`: Enforce using property signature for functions. Use this to enforce maximum correctness together with TypeScript's strict mode. | ||
- `"method"`: Enforce using method signature for functions. Use this if you aren't using TypeScript's strict mode and prefer this style. | ||
|
||
The default is `"property"`. | ||
|
||
## Rule Details | ||
|
||
Examples of **incorrect** code with `property` option. | ||
|
||
```ts | ||
interface T1 { | ||
func(arg: string): number; | ||
} | ||
type T2 = { | ||
func(arg: boolean): void; | ||
}; | ||
``` | ||
|
||
Examples of **correct** code with `property` option. | ||
|
||
```ts | ||
interface T1 { | ||
func: (arg: string) => number; | ||
} | ||
type T2 = { | ||
func: (arg: boolean) => void; | ||
}; | ||
``` | ||
|
||
Examples of **incorrect** code with `method` option. | ||
|
||
```ts | ||
interface T1 { | ||
func: (arg: string) => number; | ||
} | ||
type T2 = { | ||
func: (arg: boolean) => void; | ||
}; | ||
``` | ||
|
||
Examples of **correct** code with `method` option. | ||
|
||
```ts | ||
interface T1 { | ||
func(arg: string): number; | ||
} | ||
type T2 = { | ||
func(arg: boolean): void; | ||
}; | ||
``` | ||
|
||
## When Not To Use It | ||
|
||
If you don't want to enforce a particular style for object/interface function types, and/or if you don't use `strictFunctionTypes`, then you don't need this rule. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
118 changes: 118 additions & 0 deletions
118
packages/eslint-plugin/src/rules/method-signature-style.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
import { | ||
AST_NODE_TYPES, | ||
TSESTree, | ||
} from '@typescript-eslint/experimental-utils'; | ||
import * as util from '../util'; | ||
|
||
export type Options = ['property' | 'method']; | ||
|
||
export type MessageId = 'errorMethod' | 'errorProperty'; | ||
|
||
export default util.createRule<Options, MessageId>({ | ||
name: 'method-signature-style', | ||
meta: { | ||
type: 'suggestion', | ||
docs: { | ||
description: 'Enforces using a particular method signature syntax.', | ||
category: 'Best Practices', | ||
recommended: false, | ||
}, | ||
fixable: 'code', | ||
messages: { | ||
errorMethod: | ||
'Shorthand method signature is forbidden. Use a function property instead.', | ||
errorProperty: | ||
'Function property signature is forbidden. Use a method shorthand instead.', | ||
}, | ||
schema: [ | ||
{ | ||
enum: ['property', 'method'], | ||
}, | ||
], | ||
}, | ||
defaultOptions: ['property'], | ||
|
||
create(context, [mode]) { | ||
const sourceCode = context.getSourceCode(); | ||
|
||
function getMethodKey( | ||
node: TSESTree.TSMethodSignature | TSESTree.TSPropertySignature, | ||
): string { | ||
let key = sourceCode.getText(node.key); | ||
if (node.computed) { | ||
key = `[${key}]`; | ||
} | ||
if (node.optional) { | ||
key = `${key}?`; | ||
} | ||
if (node.readonly) { | ||
key = `readonly ${key}`; | ||
} | ||
return key; | ||
} | ||
|
||
function getMethodParams( | ||
node: TSESTree.TSMethodSignature | TSESTree.TSFunctionType, | ||
): string { | ||
let params = node.params.map(node => sourceCode.getText(node)).join(', '); | ||
params = `(${params})`; | ||
if (node.typeParameters != null) { | ||
const typeParams = sourceCode.getText(node.typeParameters); | ||
params = `${typeParams}${params}`; | ||
} | ||
return params; | ||
} | ||
|
||
function getMethodReturnType( | ||
node: TSESTree.TSMethodSignature | TSESTree.TSFunctionType, | ||
): string { | ||
return sourceCode.getText(node.returnType!.typeAnnotation); | ||
} | ||
|
||
return { | ||
TSMethodSignature(methodNode): void { | ||
if (mode === 'method') { | ||
return; | ||
} | ||
|
||
context.report({ | ||
node: methodNode, | ||
messageId: 'errorMethod', | ||
fix: fixer => { | ||
const key = getMethodKey(methodNode); | ||
const params = getMethodParams(methodNode); | ||
const returnType = getMethodReturnType(methodNode); | ||
return fixer.replaceText( | ||
methodNode, | ||
`${key}: ${params} => ${returnType}`, | ||
); | ||
}, | ||
}); | ||
}, | ||
TSPropertySignature(propertyNode): void { | ||
const typeNode = propertyNode.typeAnnotation?.typeAnnotation; | ||
if (typeNode?.type !== AST_NODE_TYPES.TSFunctionType) { | ||
return; | ||
} | ||
|
||
if (mode === 'property') { | ||
return; | ||
} | ||
|
||
context.report({ | ||
node: propertyNode, | ||
messageId: 'errorProperty', | ||
fix: fixer => { | ||
const key = getMethodKey(propertyNode); | ||
const params = getMethodParams(typeNode); | ||
const returnType = getMethodReturnType(typeNode); | ||
return fixer.replaceText( | ||
propertyNode, | ||
`${key}${params}: ${returnType}`, | ||
); | ||
}, | ||
}); | ||
}, | ||
}; | ||
}, | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This will remove all comments inside the type.
For the most part I don't have problems with a fixer removing comments in weird places if it makes the fixer simpler and easier to maintain.
But one place we should accept and support comments is around parameters, eg:
So I would suggest converting this to something like:
That way we preserve the "expected" comments
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 just did that, but it only works if there's at least one argument. If the argument list is
(/* comment */)
then the comment is lost. The problem is that there's no way to find the paren tokens because the method node's "params" property is just an empty array. This is contrary to "typeParams" which is a separate AST node so I can justgetText
it even if it's empty.