From c9c6ef52a7e3d374a330d39126d895986cf79ecd Mon Sep 17 00:00:00 2001 From: Nathan Woltman Date: Mon, 21 Aug 2017 20:43:38 -0400 Subject: [PATCH] Fix `isFirstNested` util to handle shared-line comments --- .../__tests__/index.js | 9 +++ .../__tests__/index.js | 16 ++++++ .../__tests__/index.js | 11 ++++ .../__tests__/index.js | 11 ++++ .../rule-empty-line-before/__tests__/index.js | 14 +++++ lib/rules/rule-empty-line-before/index.js | 3 +- lib/utils/__tests__/isFirstNested.test.js | 57 +++++++++++++++++++ lib/utils/isFirstNested.js | 45 +++++++++++++-- 8 files changed, 160 insertions(+), 6 deletions(-) diff --git a/lib/rules/at-rule-empty-line-before/__tests__/index.js b/lib/rules/at-rule-empty-line-before/__tests__/index.js index d91d61aa9b..1fcd8129ab 100644 --- a/lib/rules/at-rule-empty-line-before/__tests__/index.js +++ b/lib/rules/at-rule-empty-line-before/__tests__/index.js @@ -345,6 +345,10 @@ testRule( accept: [ { code: "a {\n @mixin foo;\n color: pink;\n}" + }, + { + code: "a { /* comment */\n @mixin foo;\n color: pink;\n}", + description: "shared-line comment" } ], @@ -358,6 +362,11 @@ testRule( code: "a {\n\n @mixin foo;\n color: pink;\n}", fixed: "a {\n @mixin foo;\n color: pink;\n}", message: messages.rejected + }, + { + code: "a {/* comment */\n\n @mixin foo;\n color: pink;\n}", + fixed: "a {/* comment */\n @mixin foo;\n color: pink;\n}", + message: messages.rejected } ] }) diff --git a/lib/rules/comment-empty-line-before/__tests__/index.js b/lib/rules/comment-empty-line-before/__tests__/index.js index 5c5d7561e3..be68439054 100644 --- a/lib/rules/comment-empty-line-before/__tests__/index.js +++ b/lib/rules/comment-empty-line-before/__tests__/index.js @@ -103,6 +103,12 @@ testRule( { code: "a {\n /* comment */\n color: pink;\n}", description: "first-nested without empty line before" + }, + { + code: + "a { /* shared-line comment */\n /* comment */\n color: pink;\n}", + description: + "first-nested without empty line before and shared-line comment" } ], @@ -114,6 +120,16 @@ testRule( message: messages.rejected, line: 3, column: 3 + }, + { + code: + "a { /* shared-line comment */\n\n /* comment */\n color: pink;\n}", + fixed: + "a { /* shared-line comment */\n /* comment */\n color: pink;\n}", + description: "first-nested with empty line before", + message: messages.rejected, + line: 3, + column: 3 } ] }) diff --git a/lib/rules/custom-property-empty-line-before/__tests__/index.js b/lib/rules/custom-property-empty-line-before/__tests__/index.js index e7cbe696db..cc5cc8a287 100644 --- a/lib/rules/custom-property-empty-line-before/__tests__/index.js +++ b/lib/rules/custom-property-empty-line-before/__tests__/index.js @@ -171,6 +171,10 @@ testRule(rule, { }, { code: "a {\r\n --custom-prop: value;\r\n}" + }, + { + code: "a { /* comment */\n --custom-prop: value;\n}", + description: "shared-line comment" } ], @@ -188,6 +192,13 @@ testRule(rule, { message: messages.rejected, line: 3, column: 2 + }, + { + code: "a { /* comment*/\n\n --custom-prop: value;\n}", + fixed: "a { /* comment*/\n --custom-prop: value;\n}", + message: messages.rejected, + line: 3, + column: 2 } ] }); diff --git a/lib/rules/declaration-empty-line-before/__tests__/index.js b/lib/rules/declaration-empty-line-before/__tests__/index.js index 1ca69d9923..3a1ce0b0f9 100644 --- a/lib/rules/declaration-empty-line-before/__tests__/index.js +++ b/lib/rules/declaration-empty-line-before/__tests__/index.js @@ -365,6 +365,10 @@ testRule(rule, { }, { code: "a {\r\n top: 15px;\r\n}" + }, + { + code: "a { /* comment */\n top: 15px;\n}", + description: "shared-line comment" } ], @@ -382,6 +386,13 @@ testRule(rule, { message: messages.rejected, line: 3, column: 2 + }, + { + code: "a { /* comment */\n\n top: 15px;\n}", + fixed: "a { /* comment */\n top: 15px;\n}", + message: messages.rejected, + line: 3, + column: 2 } ] }); diff --git a/lib/rules/rule-empty-line-before/__tests__/index.js b/lib/rules/rule-empty-line-before/__tests__/index.js index efed8758e9..a8256ef1c9 100644 --- a/lib/rules/rule-empty-line-before/__tests__/index.js +++ b/lib/rules/rule-empty-line-before/__tests__/index.js @@ -235,6 +235,10 @@ testRule(rule, { code: "@media {\r\n a {}\r\n\r\n}", description: "CRLF" }, + { + code: "@media { /* comment */\n a {}\n\n}", + description: "shared-line comment boog" + }, { code: "@media {\n a {}\n\n b{}\n\n}" }, @@ -268,6 +272,16 @@ testRule(rule, { fixed: "@media {\n a {}\n}", message: messages.rejected }, + { + code: "@media { /* comment */\n\n a {}\n}", + fixed: "@media { /* comment */\n a {}\n}", + message: messages.rejected + }, + { + code: "@media { /* comment */\n\n a {}\n}", + fixed: "@media { /* comment */\n a {}\n}", + message: messages.rejected + }, { code: "@media {\n\n a {}\n\n b{}\n}", fixed: "@media {\n a {}\n\n b{}\n}", diff --git a/lib/rules/rule-empty-line-before/index.js b/lib/rules/rule-empty-line-before/index.js index beef268e4e..9b263903e5 100644 --- a/lib/rules/rule-empty-line-before/index.js +++ b/lib/rules/rule-empty-line-before/index.js @@ -2,6 +2,7 @@ const addEmptyLineBefore = require("../../utils/addEmptyLineBefore"); const hasEmptyLine = require("../../utils/hasEmptyLine"); +const isFirstNested = require("../../utils/isFirstNested"); const isSingleLineString = require("../../utils/isSingleLineString"); const isStandardSyntaxRule = require("../../utils/isStandardSyntaxRule"); const optionsMatches = require("../../utils/optionsMatches"); @@ -86,7 +87,7 @@ const rule = function(expectation, options, context) { // Optionally reverse the expectation for the first nested node if ( optionsMatches(options, "except", "first-nested") && - rule === rule.parent.first + isFirstNested(rule) ) { expectEmptyLineBefore = !expectEmptyLineBefore; } diff --git a/lib/utils/__tests__/isFirstNested.test.js b/lib/utils/__tests__/isFirstNested.test.js index 00922f7921..034c608aaa 100644 --- a/lib/utils/__tests__/isFirstNested.test.js +++ b/lib/utils/__tests__/isFirstNested.test.js @@ -17,6 +17,9 @@ describe("isFirstNested", () => { @media (min-width: 0px) { a { color: 'pink'; } } + @media (min-width: 0px) { /* comment */ + a { color: 'pink'; } + } `); root.walkRules(rule => { @@ -29,6 +32,9 @@ describe("isFirstNested", () => { a { @include foo; } + b { /* comment */ + @include foo; + } `); root.walkAtRules(atRule => { @@ -36,11 +42,29 @@ describe("isFirstNested", () => { }); }); + it("returns true with the first-nested declaration", () => { + const root = postcss.parse(` + a { + color: pink; + } + b { /* comment */ + color: pink; + } + `); + + root.walkDecls(atRule => { + expect(isFirstNested(atRule)).toBe(true); + }); + }); + it("returns true with first-nested non-statement", () => { const root = postcss.parse(` a { /* comment */ } + b { /* shared-line comment */ + /* comment */ + } `); root.walkComments(comment => { @@ -54,6 +78,10 @@ describe("isFirstNested", () => { a { color: 'pink'; } b { color: 'pink'; } } + @media (min-width: 0px) { + /* comment */ + b { color: 'pink'; } + } `); root.walkRules("b", rule => { @@ -67,10 +95,39 @@ describe("isFirstNested", () => { @include foo; @expect bar; } + b { + /* comment */ + @expect bar; + } `); root.walkAtRules("expect", atRule => { expect(isFirstNested(atRule)).toBe(false); }); }); + + it("returns false with not-first-nested declaration", () => { + const root = postcss.parse(` + a { + font-size: 0; + color: pink; + } + b { + /* comment */ + color: pink; + } + c { /* comment */ /* comment */ + /* comment */ + color: pink; + } + d { /* shared-line/multi-line + comment */ + color: pink; + } + `); + + root.walkDecls("color", atRule => { + expect(isFirstNested(atRule)).toBe(false); + }); + }); }); diff --git a/lib/utils/isFirstNested.js b/lib/utils/isFirstNested.js index d5c33693b5..3bda9defa2 100644 --- a/lib/utils/isFirstNested.js +++ b/lib/utils/isFirstNested.js @@ -4,9 +4,44 @@ module.exports = function(statement /*: postcss$node*/) /*: boolean*/ { const parentNode = statement.parent; - return ( - parentNode !== undefined && - parentNode.type !== "root" && - statement === parentNode.first - ); + if (parentNode === undefined || parentNode.type === "root") { + return false; + } + + if (statement === parentNode.first) { + return true; + } + + /* + * Search for the statement in the parent's nodes, ignoring comment + * nodes on the same line as the parent's opening brace. + */ + + const parentNodes = parentNode.nodes; + const firstNode = parentNodes[0]; + + if (firstNode.type !== "comment" || firstNode.raws.before.includes("\n")) { + return false; + } + + const openingBraceLine = firstNode.source.start.line; + + if (openingBraceLine !== firstNode.source.end.line) { + return false; + } + + for (let i = 1; i < parentNodes.length; i++) { + const node = parentNodes[i]; + + if (node === statement) { + return true; + } + + if (node.type !== "comment" || node.source.end.line !== openingBraceLine) { + return false; + } + } + + /* istanbul ignore next: Should always return in the loop */ + return false; };