New: multiline-comment-style rule (fixes #8320) #9389

aladdin-add · 2017-10-04T22:38:46Z

What is the purpose of this pull request? (put an "X" next to item)

[x] New rule (template)

What changes did you make? (Give an overview)
fixes #8320

Is there anything you'd like reviewers to focus on?

This code is currently messy and incomplete. There are also some edge cases that the rule's design needs to address (e.g. allowing "banner" comments which are created from several consecutive line comments).

not-an-aardvark

Thanks for the PR! This mostly looks good -- I just noticed a small bug.

not-an-aardvark · 2017-10-10T20:04:01Z

tests/lib/rules/multiline-comment-style.js

+            `,
+            options: ["bare-block"],
+            errors: [{ message: EXPECTED_BLOCK_ERROR, line: 2 }]
+        }


Can you add a test where a single-line comment contains */? I think this will currently cause invalid syntax.

/* eslint multiline-comment-style: [error, bare-block] */ // foo */ // bar

not-an-aardvark · 2017-10-10T20:17:11Z

lib/rules/multiline-comment-style.js

+                                    : MISSING_STAR_ERROR,
+                                fix(fixer) {
+
+                                    // TODO: Make this more readable, possibly by splitting it into two separate cases.


Is this comment still needed?

not-an-aardvark · 2017-10-10T20:20:53Z

lib/rules/multiline-comment-style.js

+                            },
+                            message: EXPECTED_BLOCK_ERROR,
+                            fix(fixer) {
+                                return fixer.replaceTextRange([commentGroup[0].range[0], commentGroup[commentGroup.length - 1].range[1]], convertToBlock(commentGroup[0], commentLines.filter(line => line)));


Nitpick: Can you split this onto multiple lines?

return fixer.replaceTextRange( [commentGroup[0].range[0], commentGroup[commentGroup.length - 1].range[1]], convertToBlock(commentGroup[0], commentLines.filter(line => line)) );

(As a separate note, maybe we should enable max-len on the codebase.)

not-an-aardvark · 2017-10-10T20:22:43Z

lib/rules/multiline-comment-style.js

+                    // 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).slice(1, -1);


Why is .slice(1, -1) used here? It seems like this will omit the last line if someone does this:

/* foo * bar */

this is to skip the first line and last line in some cases like this:

/* * foo * bar */

but seems not ideal. 😂

not-an-aardvark · 2017-10-10T20:39:23Z

lib/rules/multiline-comment-style.js

+                        },
+                        message: EXPECTED_LINES_ERROR,
+                        fix(fixer) {
+                            return fixer.replaceTextRange(block.range, convertToSeparateLines(block, commentLines.filter(line => line)));


Nitpick: This could be

return fixer.replaceText(block, convertToSeparateLines(block, commentLines.filter(line => line)));

not-an-aardvark · 2017-10-10T20:41:10Z

lib/rules/multiline-comment-style.js

+                                },
+                                message: EXPECTED_BLOCK_ERROR,
+                                fix(fixer) {
+                                    return fixer.replaceTextRange(block.range, convertToBlock(block, commentLines.filter(line => line)));


Nitpick: This could be

return fixer.replaceText(block, convertToBlock(block, commentLines.filter(line => line)));

(Maybe we should create a new rule in eslint-plugin-eslint-plugin for it 😃)

not-an-aardvark · 2017-10-10T20:44:56Z

tests/lib/rules/multiline-comment-style.js

+        },
+        {
+            code: `
+                /* this blockk


Nitpick: There is a typo here (this should just say block).

not-an-aardvark · 2017-10-10T21:04:06Z

lib/rules/multiline-comment-style.js

+                        });
+                    }
+
+                    // TODO: clean this up


Is this comment still needed?

Squashed commits: [914bea1] fix: fixer would cause invalid syntax. [a4dd76c] Fix: isJSDoc checking each line starts with "*". [e16985a] prefer fixer.replaceText(). .

eslintbot · 2017-10-12T09:57:38Z

LGTM

not-an-aardvark

Looks good to me. Thanks!

kaicataldo

This is looking great! Just a few comments/questions.

kaicataldo · 2017-10-13T21:37:16Z

lib/rules/multiline-comment-style.js

+        return {
+            Program() {
+                return sourceCode.getAllComments()
+                    .filter(comment => comment.type !== "Shebang")


Nitpick: Is there a reason all these checks can't happen in one .filter() to cut down on the number of times we have to iterate over the comments array?

I don't feel strong about this. will update if you like it~ 😄

kaicataldo · 2017-10-13T21:50:11Z

lib/rules/multiline-comment-style.js

+
+                    // disallows consecutive line comments in favor of using a block comment.
+                    if (commentGroup[0].type === "Line" && commentLines.length > 1 &&
+                        !commentLines.some(value => value.includes("*/"))) {


Why is this check necessary? Example of something that would be ignored that I'm not sure should be:

// Here's some regex /\w*/ in // a comment.

Answered by #9389 (comment)

kaicataldo · 2017-10-13T21:55:13Z

lib/rules/multiline-comment-style.js

+            "starred-block"(commentGroup) {
+                const commentLines = getCommentLines(commentGroup);
+
+                if (commentLines.some(value => value.includes("*/"))) {


Why is this check necessary? Example of something that would be ignored that I'm not sure should be:

// Here's some regex /\w*/ in // a comment.

*/ cannot exsist in a block comment, e.g.

/* Here's some regex /\w*/ in a comment. */

Unless I'm mistaken, can't commentLines also include Line comments? Or maybe a better question is this: what would happen for the example I listed above? Apologies if I'm misreading or misunderstanding something!

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 */ can't be converted to block comments, so it doesn't make sense for the rule to report them.

Ah, right - got it! Thanks!

kaicataldo · 2017-10-13T22:00:18Z

lib/rules/multiline-comment-style.js

+        function isJSDoc(commentGroup) {
+            const lines = commentGroup[0].value.split(astUtils.LINEBREAK_MATCHER);
+
+            return commentGroup[0].type === "Block" &&


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

kaicataldo · 2017-10-14T18:27:00Z

Thanks for contributing to ESLint!

aladdin-add force-pushed the issue8320 branch from 672bdad to ddd95fa Compare October 5, 2017 07:56

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules labels Oct 5, 2017

not-an-aardvark and others added 2 commits October 6, 2017 22:07

New: multiline-comment-style rule (fixes eslint#8320)

11f99b7

This code is currently messy and incomplete. There are also some edge cases that the rule's design needs to address (e.g. allowing "banner" comments which are created from several consecutive line comments).

Chore: add multiline-comment-style in eslint:recommended.

90c85f4

aladdin-add force-pushed the issue8320 branch from ddd95fa to 19eee79 Compare October 6, 2017 14:07

finished :(

4d28db5

aladdin-add force-pushed the issue8320 branch from 19eee79 to 4d28db5 Compare October 6, 2017 14:58

aladdin-add changed the title ~~WIP: New: multiline-comment-style rule (fixes #8320)~~ New: multiline-comment-style rule (fixes #8320) Oct 6, 2017

not-an-aardvark suggested changes Oct 10, 2017

View reviewed changes

薛定谔的猫 and others added 2 commits October 11, 2017 20:13

Fix: review suggestions

1ae221a

Squashed commits: [914bea1] fix: fixer would cause invalid syntax. [a4dd76c] Fix: isJSDoc checking each line starts with "*". [e16985a] prefer fixer.replaceText(). .

Update multiline-comment-style.md

2d3fc80

not-an-aardvark approved these changes Oct 13, 2017

View reviewed changes

eslint deleted a comment from eslintbot Oct 13, 2017

kaicataldo reviewed Oct 13, 2017

View reviewed changes

ilyavolodin approved these changes Oct 14, 2017

View reviewed changes

ilyavolodin merged commit 8eb4aae into eslint:master Oct 14, 2017

not-an-aardvark mentioned this pull request Oct 20, 2017

Chore: enable max-len. #9414

Merged

aladdin-add deleted the issue8320 branch April 1, 2018 13:20

eslint-deprecated bot locked and limited conversation to collaborators Apr 13, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: multiline-comment-style rule (fixes #8320) #9389

New: multiline-comment-style rule (fixes #8320) #9389

aladdin-add commented Oct 4, 2017

not-an-aardvark left a comment

not-an-aardvark Oct 10, 2017

not-an-aardvark Oct 10, 2017

not-an-aardvark Oct 10, 2017

not-an-aardvark Oct 10, 2017

aladdin-add Oct 11, 2017

not-an-aardvark Oct 10, 2017

not-an-aardvark Oct 10, 2017

aladdin-add Oct 11, 2017

not-an-aardvark Oct 10, 2017

not-an-aardvark Oct 10, 2017

eslintbot commented Oct 12, 2017

not-an-aardvark left a comment

kaicataldo left a comment

kaicataldo Oct 13, 2017

aladdin-add Oct 14, 2017

kaicataldo Oct 13, 2017

kaicataldo Oct 14, 2017 •

edited

kaicataldo Oct 13, 2017

aladdin-add Oct 14, 2017 •

edited

kaicataldo Oct 14, 2017

not-an-aardvark Oct 14, 2017

kaicataldo Oct 14, 2017

kaicataldo Oct 13, 2017

kaicataldo commented Oct 14, 2017

New: multiline-comment-style rule (fixes #8320) #9389

New: multiline-comment-style rule (fixes #8320) #9389

Conversation

aladdin-add commented Oct 4, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Oct 12, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aladdin-add Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Oct 14, 2017

kaicataldo Oct 14, 2017 •

edited

aladdin-add Oct 14, 2017 •

edited