Fix: Curly rule doesn't account for leading comment (fixes #7538) #7539

wwwillchen · 2016-11-05T01:04:41Z

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

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #7538

What changes did you make? (Give an overview)
Fixes the curly rule under the "multi-or-nest" setting, includes tests, and updates docs with an example

Is there anything you'd like reviewers to focus on?
This only addresses leading comments and not trailing comments, but I think trailing comment is a bit different because the intent is more ambiguous. For example:

if (foo)
  baz();
  // line of comment
bar();

It's unclear if the comment is describing the line above or mis-indented and describing the line below. In my experience, intentional trailing comments are rare.

jsf-clabot · 2016-11-05T01:04:42Z

All committers have signed the CLA.

mention-bot · 2016-11-05T01:04:42Z

@wwwillchen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @not-an-aardvark and @nzakas to be potential reviewers.

eslintbot · 2016-11-05T01:04:42Z

LGTM

not-an-aardvark

LGTM, thanks!

mysticatea · 2016-11-05T06:32:30Z

lib/rules/curly.js

@@ -290,7 +290,7 @@ module.exports = {
                }
            } else if (multiOrNest) {
                if (hasBlock && body.body.length === 1 && isOneLiner(body.body[0])) {
-                    expected = false;
+                    expected = !!(body.body[0].leadingComments && body.body[0].leadingComments.length);


I think we should change the behavior of isOneLiner function?

I tried making my modification in isOneLiner but it caused unit tests to fail. I think the use of isOneLiner in L294 makes modifying the isOneLiner function different than my current change.

kaicataldo · 2016-11-05T20:46:25Z

lib/rules/curly.js

@@ -290,7 +290,7 @@ module.exports = {
                }
            } else if (multiOrNest) {
                if (hasBlock && body.body.length === 1 && isOneLiner(body.body[0])) {
-                    expected = false;
+                    expected = !!(body.body[0].leadingComments && body.body[0].leadingComments.length);


Can we use sourceCode.getComments(body.body[0]).leading instead of body.body[0].leadingComments?

We have a few other cases of incorrectly using the leadingComments property in the codebase, but as we are actively moving towards calculating location of comments relative to nodes in ESLint instead of the parser, this will save us having to change this down the road. Thanks!

eslintbot · 2016-11-07T18:43:16Z

LGTM

wwwillchen

Thanks for the feedback. Please take another look.

wwwillchen · 2016-11-07T18:44:46Z

lib/rules/curly.js

@@ -290,7 +290,7 @@ module.exports = {
                }
            } else if (multiOrNest) {
                if (hasBlock && body.body.length === 1 && isOneLiner(body.body[0])) {
-                    expected = false;
+                    expected = !!(body.body[0].leadingComments && body.body[0].leadingComments.length);


I tried making my modification in isOneLiner but it caused unit tests to fail. I think the use of isOneLiner in L294 makes modifying the isOneLiner function different than my current change.

wwwillchen · 2016-11-07T18:45:02Z

lib/rules/curly.js

@@ -290,7 +290,7 @@ module.exports = {
                }
            } else if (multiOrNest) {
                if (hasBlock && body.body.length === 1 && isOneLiner(body.body[0])) {
-                    expected = false;
+                    expected = !!(body.body[0].leadingComments && body.body[0].leadingComments.length);


kaicataldo · 2016-11-07T19:25:43Z

lib/rules/curly.js

-                    expected = false;
+                    const leadingComments = sourceCode.getComments(body.body[0]).leading;
+
+                    expected = !!(leadingComments && leadingComments.length);


This check should be able to just be expected = !!leadingComments.length now, since sourceCode.getComments() will return an empty array if no leading comments are found

eslintbot · 2016-11-07T20:05:24Z

LGTM

nzakas

Overall LGTM, just one small nitpick.

nzakas · 2016-11-08T21:38:16Z

lib/rules/curly.js

-                    expected = false;
+                    const leadingComments = sourceCode.getComments(body.body[0]).leading;
+
+                    expected = !!leadingComments.length;


Can you use leadingComments.length > 0 instead? I prefer to be explicit about the expectation here.

eslintbot · 2016-11-08T21:54:35Z

LGTM

nzakas

LGTM

Fix: Curly rule doesn't account for leading comment (fixes eslint#7538)

1bf8a2c

not-an-aardvark approved these changes Nov 5, 2016

View reviewed changes

mysticatea reviewed Nov 5, 2016

View reviewed changes

kaicataldo reviewed Nov 5, 2016

View reviewed changes

PR feedback

b0191d7

wwwillchen commented Nov 7, 2016

View reviewed changes

wwwillchen added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Nov 7, 2016

kaicataldo reviewed Nov 7, 2016

View reviewed changes

simplify check

a2452be

nzakas approved these changes Nov 8, 2016

View reviewed changes

check .length > 0

16d626f

nzakas approved these changes Nov 8, 2016

View reviewed changes

nzakas merged commit 0d60db7 into eslint:master Nov 9, 2016

wwwillchen mentioned this pull request Nov 14, 2016

Docs: Fix example for curly "multi-or-nest" option #7597

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 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 Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Curly rule doesn't account for leading comment (fixes #7538) #7539

Fix: Curly rule doesn't account for leading comment (fixes #7538) #7539

wwwillchen commented Nov 5, 2016

jsf-clabot commented Nov 5, 2016 •

edited

mention-bot commented Nov 5, 2016

eslintbot commented Nov 5, 2016

not-an-aardvark left a comment

mysticatea Nov 5, 2016

wwwillchen Nov 7, 2016

kaicataldo Nov 5, 2016 •

edited

wwwillchen Nov 7, 2016

eslintbot commented Nov 7, 2016

wwwillchen left a comment

wwwillchen Nov 7, 2016

wwwillchen Nov 7, 2016

kaicataldo Nov 7, 2016 •

edited

wwwillchen Nov 7, 2016

eslintbot commented Nov 7, 2016

nzakas left a comment

nzakas Nov 8, 2016

wwwillchen Nov 8, 2016

eslintbot commented Nov 8, 2016

nzakas left a comment

Fix: Curly rule doesn't account for leading comment (fixes #7538) #7539

Fix: Curly rule doesn't account for leading comment (fixes #7538) #7539

Conversation

wwwillchen commented Nov 5, 2016

jsf-clabot commented Nov 5, 2016 • edited

mention-bot commented Nov 5, 2016

eslintbot commented Nov 5, 2016

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

kaicataldo Nov 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Nov 7, 2016

wwwillchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Nov 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Nov 7, 2016

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Nov 8, 2016

nzakas left a comment

Choose a reason for hiding this comment

jsf-clabot commented Nov 5, 2016 •

edited

kaicataldo Nov 5, 2016 •

edited

kaicataldo Nov 7, 2016 •

edited