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): [block-spacing] extending base rule for TS related blocks #6195
Merged
bradzacher
merged 25 commits into
typescript-eslint:main
from
omril1:feat/issue5990-extend-base-block-spacing
Feb 13, 2023
Merged
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
14ec73d
feat/issue5990-extend-base-block-spacing
f393928
for history
0067b46
fixes
7e8b540
cleanup
3cd0d6f
with a comment for the option
590d9f8
support enums as well
ab64e29
template string
65fa0e6
fix doc
d38b80e
fix doc
dbe273c
typescript -> TypeScript
2496815
add missing things
21a44fa
CR: remove jsdoc types
82ec70d
rename typeDeclaration -> typeDeclarations
cef520c
more valid cases with comments
415733a
invalid cases with block comments
725d8a9
more concise
682503d
Merge branch 'main' into feat/issue5990-extend-base-block-spacing
6fd6ffb
fix comment
7f9b654
Merge remote-tracking branch 'upstream/main' into feat/issue5990-exte…
fe35cab
Merge branch 'main' into feat/issue5990-extend-base-block-spacing
679e13f
Merge branch 'main' into feat/issue5990-extend-base-block-spacing
e215ca0
Merge remote-tracking branch 'upstream/main' into feat/issue5990-exte…
9ccdd26
Merge branch 'main' into feat/issue5990-extend-base-block-spacing
c149a12
Merge remote-tracking branch 'upstream/main' into feat/issue5990-exte…
b6b1140
fix lint error
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
--- | ||
description: 'Disallow or enforce spaces inside of blocks after opening block and before closing block.' | ||
--- | ||
|
||
> 🛑 This file is source code, not the primary documentation location! 🛑 | ||
> | ||
> See **https://typescript-eslint.io/rules/block-spacing** for documentation. | ||
|
||
## Examples | ||
|
||
This rule extends the base [`eslint/block-spacing`](https://eslint.org/docs/rules/block-spacing) rule. | ||
This version adds support for TypeScript related blocks (interfaces, object type literals and enums). |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,166 @@ | ||
import type { TSESTree } from '@typescript-eslint/utils'; | ||
import { AST_TOKEN_TYPES } from '@typescript-eslint/utils'; | ||
|
||
import * as util from '../util'; | ||
import { getESLintCoreRule } from '../util/getESLintCoreRule'; | ||
|
||
const baseRule = getESLintCoreRule('block-spacing'); | ||
|
||
export type Options = util.InferOptionsTypeFromRule<typeof baseRule>; | ||
export type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>; | ||
|
||
export default util.createRule<Options, MessageIds>({ | ||
name: 'block-spacing', | ||
meta: { | ||
type: 'layout', | ||
docs: { | ||
description: | ||
'Disallow or enforce spaces inside of blocks after opening block and before closing block', | ||
recommended: false, | ||
extendsBaseRule: true, | ||
}, | ||
fixable: 'whitespace', | ||
hasSuggestions: baseRule.meta.hasSuggestions, | ||
schema: baseRule.meta.schema, | ||
messages: baseRule.meta.messages, | ||
}, | ||
defaultOptions: ['always'], | ||
|
||
create(context, [whenToApplyOption]) { | ||
const sourceCode = context.getSourceCode(); | ||
const baseRules = baseRule.create(context); | ||
const always = whenToApplyOption !== 'never'; | ||
const messageId = always ? 'missing' : 'extra'; | ||
/** | ||
* Gets the open brace token from a given node. | ||
* @returns The token of the open brace. | ||
*/ | ||
function getOpenBrace( | ||
node: TSESTree.TSEnumDeclaration, | ||
): TSESTree.PunctuatorToken { | ||
// guaranteed for enums | ||
// This is the only change made here from the base rule | ||
return sourceCode.getFirstToken(node, { | ||
filter: token => | ||
token.type === AST_TOKEN_TYPES.Punctuator && token.value == '{', | ||
}) as TSESTree.PunctuatorToken; | ||
} | ||
|
||
/** | ||
* Checks whether or not: | ||
* - given tokens are on same line. | ||
* - there is/isn't a space between given tokens. | ||
* @param left A token to check. | ||
* @param right The token which is next to `left`. | ||
* @returns | ||
* When the option is `"always"`, `true` if there are one or more spaces between given tokens. | ||
* When the option is `"never"`, `true` if there are not any spaces between given tokens. | ||
* If given tokens are not on same line, it's always `true`. | ||
*/ | ||
function isValid(left: TSESTree.Token, right: TSESTree.Token): boolean { | ||
return ( | ||
!util.isTokenOnSameLine(left, right) || | ||
sourceCode.isSpaceBetween!(left, right) === always | ||
); | ||
} | ||
|
||
/** | ||
* Checks and reports invalid spacing style inside braces. | ||
* @param {ASTNode} node A BlockStatement/StaticBlock/SwitchStatement node to check. | ||
* @returns | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
function checkSpacingInsideBraces(node: TSESTree.TSEnumDeclaration): void { | ||
// Gets braces and the first/last token of content. | ||
const openBrace = getOpenBrace(node); | ||
const closeBrace = sourceCode.getLastToken(node)!; | ||
const firstToken = sourceCode.getTokenAfter(openBrace, { | ||
includeComments: true, | ||
})!; | ||
const lastToken = sourceCode.getTokenBefore(closeBrace, { | ||
includeComments: true, | ||
})!; | ||
|
||
// Skip if the node is invalid or empty. | ||
if ( | ||
openBrace.type !== AST_TOKEN_TYPES.Punctuator || | ||
openBrace.value !== '{' || | ||
closeBrace.type !== AST_TOKEN_TYPES.Punctuator || | ||
closeBrace.value !== '}' || | ||
firstToken === closeBrace | ||
) { | ||
return; | ||
} | ||
|
||
// Skip line comments for option never | ||
if (!always && firstToken.type === AST_TOKEN_TYPES.Line) { | ||
return; | ||
} | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Check. | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!isValid(openBrace, firstToken)) { | ||
let loc = openBrace.loc; | ||
|
||
if (messageId === 'extra') { | ||
loc = { | ||
start: openBrace.loc.end, | ||
end: firstToken.loc.start, | ||
}; | ||
} | ||
|
||
context.report({ | ||
node, | ||
loc, | ||
messageId, | ||
data: { | ||
location: 'after', | ||
token: openBrace.value, | ||
}, | ||
fix(fixer) { | ||
if (always) { | ||
return fixer.insertTextBefore(firstToken, ' '); | ||
} | ||
|
||
return fixer.removeRange([openBrace.range[1], firstToken.range[0]]); | ||
}, | ||
}); | ||
} | ||
if (!isValid(lastToken, closeBrace)) { | ||
let loc = closeBrace.loc; | ||
|
||
if (messageId === 'extra') { | ||
loc = { | ||
start: lastToken.loc.end, | ||
end: closeBrace.loc.start, | ||
}; | ||
} | ||
context.report({ | ||
node, | ||
loc, | ||
messageId, | ||
data: { | ||
location: 'before', | ||
token: closeBrace.value, | ||
}, | ||
fix(fixer) { | ||
if (always) { | ||
return fixer.insertTextAfter(lastToken, ' '); | ||
} | ||
|
||
return fixer.removeRange([lastToken.range[1], closeBrace.range[0]]); | ||
}, | ||
}); | ||
} | ||
} | ||
return { | ||
...baseRules, | ||
|
||
// This code worked "out of the box" for interface and type literal | ||
// Enums were very close to match as well, the only reason they it not is that was that enums don't have a body node in the parser | ||
// So the opening brace punctuator starts in the middle of the node | ||
// `getFirstToken` in the base rule did not filter for the first opening brace punctuator | ||
TSInterfaceBody: baseRules.BlockStatement as never, | ||
TSTypeLiteral: baseRules.BlockStatement as never, | ||
TSEnumDeclaration: checkSpacingInsideBraces, | ||
Comment on lines
+158
to
+160
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept the base rule for interface and type literal, though changing to |
||
}; | ||
}, | ||
}); |
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
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,92 @@ | ||
import { AST_NODE_TYPES } from '@typescript-eslint/utils'; | ||
|
||
import rule from '../../src/rules/block-spacing'; | ||
import type { InvalidTestCase, ValidTestCase } from '../RuleTester'; | ||
import { RuleTester } from '../RuleTester'; | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: '@typescript-eslint/parser', | ||
}); | ||
|
||
type InvalidBlockSpacingTestCase = InvalidTestCase< | ||
'missing' | 'extra', | ||
['always' | 'never'] | ||
>; | ||
|
||
const options = ['always', 'never'] as const; | ||
const typeDeclaration = [ | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
nodeType: AST_NODE_TYPES.TSInterfaceBody, | ||
stringPrefix: 'interface Foo ', | ||
}, | ||
{ | ||
nodeType: AST_NODE_TYPES.TSTypeLiteral, | ||
stringPrefix: 'type Foo = ', | ||
}, | ||
{ | ||
nodeType: AST_NODE_TYPES.TSEnumDeclaration, | ||
stringPrefix: 'enum Foo ', | ||
}, | ||
{ | ||
nodeType: AST_NODE_TYPES.TSEnumDeclaration, | ||
stringPrefix: 'const enum Foo ', | ||
}, | ||
]; | ||
const emptyBlocks = ['{}', '{ }']; | ||
const singlePropertyBlocks = ['{bar: true}', '{ bar: true }']; | ||
|
||
ruleTester.run('block-spacing', rule, { | ||
valid: [ | ||
// Empty blocks don't apply | ||
...options.flatMap(option => | ||
typeDeclaration.flatMap(typeDec => | ||
emptyBlocks.map<ValidTestCase<['always' | 'never']>>(blockType => ({ | ||
code: typeDec.stringPrefix + blockType, | ||
options: [option], | ||
})), | ||
), | ||
), | ||
], | ||
invalid: [ | ||
...options.flatMap(option => | ||
typeDeclaration.flatMap(typeDec => { | ||
return singlePropertyBlocks.flatMap<InvalidBlockSpacingTestCase>( | ||
(blockType, blockIndex) => { | ||
// These are actually valid, so filter them out | ||
if ( | ||
(option === 'always' && blockType.startsWith('{ ')) || | ||
(option === 'never' && blockType.startsWith('{bar')) | ||
) { | ||
return []; | ||
} | ||
const reverseBlockType = singlePropertyBlocks[1 - blockIndex]; | ||
let code = `${typeDec.stringPrefix}${blockType}; /* ${option} */`; | ||
let output = `${typeDec.stringPrefix}${reverseBlockType}; /* ${option} */`; | ||
if (typeDec.nodeType === AST_NODE_TYPES.TSEnumDeclaration) { | ||
output = output.replace(':', '='); | ||
code = code.replace(':', '='); | ||
} | ||
|
||
return { | ||
code, | ||
options: [option], | ||
output, | ||
errors: [ | ||
{ | ||
type: typeDec.nodeType, | ||
messageId: option === 'always' ? 'missing' : 'extra', | ||
data: { location: 'after', token: '{' }, | ||
}, | ||
{ | ||
type: typeDec.nodeType, | ||
messageId: option === 'always' ? 'missing' : 'extra', | ||
data: { location: 'before', token: '}' }, | ||
}, | ||
], | ||
}; | ||
}, | ||
); | ||
}), | ||
), | ||
], | ||
}); |
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
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.
The only significant change from the base rule, it worked flawlessly for interface and object type literal.
Added a filter for punctuator because enums don't have a body node in the AST
https://github.com/eslint/eslint/blob/3ae0574fc78c4a2b406625e4792cb2859cb9bcb1/lib/rules/block-spacing.js#L60