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
New: multiline-comment-style rule (fixes #8320) #9389
Changes from all commits
11f99b7
90c85f4
4d28db5
1ae221a
2d3fc80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
# enforce a particular style for multiline comments (multiline-comment-style) | ||
|
||
Many style guides require a particular style for comments that span multiple lines. For example, some style guides prefer the use of a single block comment for multiline comments, whereas other style guides prefer consecutive line comments. | ||
|
||
## Rule Details | ||
|
||
This rule aims to enforce a particular style for multiline comments. | ||
|
||
### Options | ||
|
||
This rule has a string option, which can have one of the following values: | ||
|
||
* `"starred-block"` (default): Disallows consecutive line comments in favor of block comments. Additionally, requires block comments to have an aligned `*` character before each line. | ||
* `"bare-block"`: Disallows consecutive line comments in favor of block comments, and disallows block comments from having a `"*"` character before each line. | ||
* `"separate-lines"`: Disallows block comments in favor of consecutive line comments | ||
|
||
The rule always ignores directive comments such as `/* eslint-disable */`. Additionally, unless the mode is `"starred-block"`, the rule ignores JSDoc comments. | ||
|
||
Examples of **incorrect** code for this rule: | ||
|
||
```js | ||
|
||
/* eslint multiline-comment-style: ["error", "separate-lines"] */ | ||
|
||
/* This line | ||
calls foo() */ | ||
foo(); | ||
|
||
/* | ||
* This line | ||
* calls foo() | ||
*/ | ||
foo(); | ||
|
||
/* eslint multiline-comment-style: ["error", "starred-block"] */ | ||
|
||
// this line | ||
// calls foo() | ||
foo(); | ||
|
||
/* this line | ||
calls foo() */ | ||
foo(); | ||
|
||
/* this comment | ||
* is missing a newline after /* | ||
*/ | ||
|
||
/* | ||
* this comment | ||
* is missing a newline at the end */ | ||
|
||
/* | ||
* the star in this line should have a space before it | ||
*/ | ||
|
||
/* | ||
* the star on the following line should have a space before it | ||
*/ | ||
|
||
/* eslint multiline-comment-style: ["error", "bare-block"] */ | ||
|
||
// this line | ||
// calls foo() | ||
foo(); | ||
|
||
/* | ||
* this line | ||
* calls foo() | ||
*/ | ||
foo(); | ||
|
||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```js | ||
/* eslint multiline-comment-style: ["error", "separate-lines"] */ | ||
|
||
// This line | ||
// calls foo() | ||
foo(); | ||
|
||
|
||
/* eslint multiline-comment-style: ["error", "starred-block"] */ | ||
|
||
/* | ||
* this line | ||
* calls foo() | ||
*/ | ||
foo(); | ||
|
||
|
||
/* eslint multiline-comment-style: ["error", "bare-block"] */ | ||
|
||
/* this line | ||
calls foo() */ | ||
foo(); | ||
|
||
``` | ||
|
||
## When Not To Use It | ||
|
||
If you don't want to enforce a particular style for multiline comments, you can disable the rule. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,280 @@ | ||
/** | ||
* @fileoverview enforce a particular style for multiline comments | ||
* @author Teddy Katz | ||
*/ | ||
"use strict"; | ||
|
||
const astUtils = require("../ast-utils"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: "enforce a particular style for multiline comments", | ||
category: "Stylistic Issues", | ||
recommended: false | ||
}, | ||
fixable: "whitespace", | ||
schema: [{ enum: ["starred-block", "separate-lines", "bare-block"] }] | ||
}, | ||
|
||
create(context) { | ||
const sourceCode = context.getSourceCode(); | ||
const option = context.options[0] || "starred-block"; | ||
|
||
const EXPECTED_BLOCK_ERROR = "Expected a block comment instead of consecutive line comments."; | ||
const START_NEWLINE_ERROR = "Expected a linebreak after '/*'."; | ||
const END_NEWLINE_ERROR = "Expected a linebreak before '*/'."; | ||
const MISSING_STAR_ERROR = "Expected a '*' at the start of this line."; | ||
const ALIGNMENT_ERROR = "Expected this line to be aligned with the start of the comment."; | ||
const EXPECTED_LINES_ERROR = "Expected multiple line comments instead of a block comment."; | ||
|
||
//---------------------------------------------------------------------- | ||
// Helpers | ||
//---------------------------------------------------------------------- | ||
|
||
/** | ||
* Gets a list of comment lines in a group | ||
* @param {Token[]} commentGroup A group of comments, containing either multiple line comments or a single block comment | ||
* @returns {string[]} A list of comment lines | ||
*/ | ||
function getCommentLines(commentGroup) { | ||
if (commentGroup[0].type === "Line") { | ||
return commentGroup.map(comment => comment.value); | ||
} | ||
return commentGroup[0].value | ||
.split(astUtils.LINEBREAK_MATCHER) | ||
.map(line => line.replace(/^\s*\*?/, "")); | ||
} | ||
|
||
/** | ||
* Converts a comment into starred-block form | ||
* @param {Token} firstComment The first comment of the group being converted | ||
* @param {string[]} commentLinesList A list of lines to appear in the new starred-block comment | ||
* @returns {string} A representation of the comment value in starred-block form, excluding start and end markers | ||
*/ | ||
function convertToStarredBlock(firstComment, commentLinesList) { | ||
const initialOffset = sourceCode.text.slice(firstComment.range[0] - firstComment.loc.start.column, firstComment.range[0]); | ||
const starredLines = commentLinesList.map(line => `${initialOffset} *${line}`); | ||
|
||
return `\n${starredLines.join("\n")}\n${initialOffset} `; | ||
} | ||
|
||
/** | ||
* Converts a comment into separate-line form | ||
* @param {Token} firstComment The first comment of the group being converted | ||
* @param {string[]} commentLinesList A list of lines to appear in the new starred-block comment | ||
* @returns {string} A representation of the comment value in separate-line form | ||
*/ | ||
function convertToSeparateLines(firstComment, commentLinesList) { | ||
const initialOffset = sourceCode.text.slice(firstComment.range[0] - firstComment.loc.start.column, firstComment.range[0]); | ||
const separateLines = commentLinesList.map(line => `// ${line.trim()}`); | ||
|
||
return separateLines.join(`\n${initialOffset}`); | ||
} | ||
|
||
/** | ||
* Converts a comment into bare-block form | ||
* @param {Token} firstComment The first comment of the group being converted | ||
* @param {string[]} commentLinesList A list of lines to appear in the new starred-block comment | ||
* @returns {string} A representation of the comment value in bare-block form | ||
*/ | ||
function convertToBlock(firstComment, commentLinesList) { | ||
const initialOffset = sourceCode.text.slice(firstComment.range[0] - firstComment.loc.start.column, firstComment.range[0]); | ||
const blockLines = commentLinesList.map(line => line.trim()); | ||
|
||
return `/* ${blockLines.join(`\n${initialOffset} `)} */`; | ||
} | ||
|
||
/** | ||
* Check a comment is JSDoc form | ||
* @param {Token[]} commentGroup A group of comments, containing either multiple line comments or a single block comment | ||
* @returns {boolean} if commentGroup is JSDoc form, return true | ||
*/ | ||
function isJSDoc(commentGroup) { | ||
const lines = commentGroup[0].value.split(astUtils.LINEBREAK_MATCHER); | ||
|
||
return commentGroup[0].type === "Block" && | ||
/^\*\s*$/.test(lines[0]) && | ||
lines.slice(1, -1).every(line => /^\s* /.test(line)) && | ||
/^\s*$/.test(lines[lines.length - 1]); | ||
} | ||
|
||
/** | ||
* Each method checks a group of comments to see if it's valid according to the given option. | ||
* @param {Token[]} commentGroup A list of comments that appear together. This will either contain a single | ||
* block comment or multiple line comments. | ||
* @returns {void} | ||
*/ | ||
const commentGroupCheckers = { | ||
"starred-block"(commentGroup) { | ||
const commentLines = getCommentLines(commentGroup); | ||
|
||
if (commentLines.some(value => value.includes("*/"))) { | ||
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. Why is this check necessary? Example of something that would be ignored that I'm not sure should be:
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.
/* Here's some regex /\w*/ in
a comment. */ 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. Unless I'm mistaken, can't 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. This part of the rule looks at line comments and reports an error to indicate that they should be converted to block comments. However, line comments that contain 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. Ah, right - got it! Thanks! |
||
return; | ||
} | ||
|
||
if (commentGroup.length > 1) { | ||
context.report({ | ||
loc: { | ||
start: commentGroup[0].loc.start, | ||
end: commentGroup[commentGroup.length - 1].loc.end | ||
}, | ||
message: EXPECTED_BLOCK_ERROR, | ||
fix(fixer) { | ||
return fixer.replaceTextRange( | ||
[commentGroup[0].range[0], commentGroup[commentGroup.length - 1].range[1]], | ||
`/*${convertToStarredBlock(commentGroup[0], commentLines)}*/` | ||
); | ||
} | ||
}); | ||
} else { | ||
const block = commentGroup[0]; | ||
const lines = block.value.split(astUtils.LINEBREAK_MATCHER); | ||
const expectedLinePrefix = `${sourceCode.text.slice(block.range[0] - block.loc.start.column, block.range[0])} *`; | ||
|
||
if (!/^\*?\s*$/.test(lines[0])) { | ||
const start = block.value.startsWith("*") ? block.range[0] + 1 : block.range[0]; | ||
|
||
context.report({ | ||
loc: { | ||
start: block.loc.start, | ||
end: { line: block.loc.start.line, column: block.loc.start.column + 2 } | ||
}, | ||
message: START_NEWLINE_ERROR, | ||
fix: fixer => fixer.insertTextAfterRange([start, start + 2], `\n${expectedLinePrefix}`) | ||
}); | ||
} | ||
|
||
if (!/^\s*$/.test(lines[lines.length - 1])) { | ||
context.report({ | ||
loc: { | ||
start: { line: block.loc.end.line, column: block.loc.end.column - 2 }, | ||
end: block.loc.end | ||
}, | ||
message: END_NEWLINE_ERROR, | ||
fix: fixer => fixer.replaceTextRange([block.range[1] - 2, block.range[1]], `\n${expectedLinePrefix}/`) | ||
}); | ||
} | ||
|
||
for (let lineNumber = block.loc.start.line + 1; lineNumber <= block.loc.end.line; lineNumber++) { | ||
const lineText = sourceCode.lines[lineNumber - 1]; | ||
|
||
if (!lineText.startsWith(expectedLinePrefix)) { | ||
context.report({ | ||
loc: { | ||
start: { line: lineNumber, column: 0 }, | ||
end: { line: lineNumber, column: sourceCode.lines[lineNumber - 1].length } | ||
}, | ||
message: /^\s*\*/.test(lineText) | ||
? ALIGNMENT_ERROR | ||
: MISSING_STAR_ERROR, | ||
fix(fixer) { | ||
const lineStartIndex = sourceCode.getIndexFromLoc({ line: lineNumber, column: 0 }); | ||
const commentStartIndex = lineStartIndex + lineText.match(/^\s*\*? ?/)[0].length; | ||
const replacementText = lineNumber === block.loc.end.line ? expectedLinePrefix : `${expectedLinePrefix} `; | ||
|
||
return fixer.replaceTextRange([lineStartIndex, commentStartIndex], replacementText); | ||
} | ||
}); | ||
} | ||
} | ||
} | ||
}, | ||
"separate-lines"(commentGroup) { | ||
if (!isJSDoc(commentGroup) && commentGroup[0].type === "Block") { | ||
const commentLines = getCommentLines(commentGroup); | ||
const block = commentGroup[0]; | ||
|
||
context.report({ | ||
loc: { | ||
start: block.loc.start, | ||
end: { line: block.loc.start.line, column: block.loc.start.column + 2 } | ||
}, | ||
message: EXPECTED_LINES_ERROR, | ||
fix(fixer) { | ||
return fixer.replaceText(block, convertToSeparateLines(block, commentLines.filter(line => line))); | ||
} | ||
}); | ||
} | ||
}, | ||
"bare-block"(commentGroup) { | ||
if (!isJSDoc(commentGroup)) { | ||
const commentLines = getCommentLines(commentGroup); | ||
|
||
// disallows consecutive line comments in favor of using a block comment. | ||
if (commentGroup[0].type === "Line" && commentLines.length > 1 && | ||
!commentLines.some(value => value.includes("*/"))) { | ||
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. Why is this check necessary? Example of something that would be ignored that I'm not sure should be:
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. Answered by #9389 (comment) |
||
context.report({ | ||
loc: { | ||
start: commentGroup[0].loc.start, | ||
end: commentGroup[commentGroup.length - 1].loc.end | ||
}, | ||
message: EXPECTED_BLOCK_ERROR, | ||
fix(fixer) { | ||
const range = [commentGroup[0].range[0], commentGroup[commentGroup.length - 1].range[1]]; | ||
const block = convertToBlock(commentGroup[0], commentLines.filter(line => line)); | ||
|
||
return fixer.replaceTextRange(range, block); | ||
} | ||
}); | ||
} | ||
|
||
// prohibits block comments from having a * at the beginning of each line. | ||
if (commentGroup[0].type === "Block") { | ||
const block = commentGroup[0]; | ||
const lines = block.value.split(astUtils.LINEBREAK_MATCHER).filter(line => line.trim()); | ||
|
||
if (lines.length > 0 && lines.every(line => /^\s*\*/.test(line))) { | ||
context.report({ | ||
loc: { | ||
start: block.loc.start, | ||
end: { line: block.loc.start.line, column: block.loc.start.column + 2 } | ||
}, | ||
message: EXPECTED_BLOCK_ERROR, | ||
fix(fixer) { | ||
return fixer.replaceText(block, convertToBlock(block, commentLines.filter(line => line))); | ||
} | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
|
||
//---------------------------------------------------------------------- | ||
// Public | ||
//---------------------------------------------------------------------- | ||
|
||
return { | ||
Program() { | ||
return sourceCode.getAllComments() | ||
.filter(comment => comment.type !== "Shebang") | ||
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. Nitpick: Is there a reason all these checks can't happen in one 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. I don't feel strong about this. will update if you like it~ 😄 |
||
.filter(comment => !astUtils.COMMENTS_IGNORE_PATTERN.test(comment.value)) | ||
.filter(comment => { | ||
const tokenBefore = sourceCode.getTokenBefore(comment, { includeComments: true }); | ||
|
||
return !tokenBefore || tokenBefore.loc.end.line < comment.loc.start.line; | ||
}) | ||
.reduce((commentGroups, comment, index, commentList) => { | ||
if ( | ||
comment.type === "Line" && | ||
index && commentList[index - 1].type === "Line" && | ||
sourceCode.getTokenBefore(comment, { includeComments: true }) === commentList[index - 1] | ||
) { | ||
commentGroups[commentGroups.length - 1].push(comment); | ||
} else { | ||
commentGroups.push([comment]); | ||
} | ||
|
||
return commentGroups; | ||
}, []) | ||
.filter(commentGroup => !(commentGroup.length === 1 && commentGroup[0].loc.start.line === commentGroup[0].loc.end.line)) | ||
.forEach(commentGroupCheckers[option]); | ||
} | ||
}; | ||
} | ||
}; |
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 wonder if we should make extract this into ast-utils so we can reuse it?
Another place we do a similar (though less rigorous) check that we could unify: https://github.com/eslint/eslint/blob/master/lib/util/source-code.js#L295