Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix isFirstNested util to handle shared-line comments #2827

Merged
merged 1 commit into from
Sep 2, 2017
Merged

Fix isFirstNested util to handle shared-line comments #2827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/rules/at-rule-empty-line-before/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
],

Expand All @@ -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
}
]
})
Expand Down
16 changes: 16 additions & 0 deletions lib/rules/comment-empty-line-before/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
],

Expand All @@ -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
}
]
})
Expand Down
11 changes: 11 additions & 0 deletions lib/rules/custom-property-empty-line-before/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
],

Expand All @@ -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
}
]
});
Expand Down
11 changes: 11 additions & 0 deletions lib/rules/declaration-empty-line-before/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
],

Expand All @@ -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
}
]
});
Expand Down
14 changes: 14 additions & 0 deletions lib/rules/rule-empty-line-before/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
},
Expand Down Expand Up @@ -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}",
Expand Down
3 changes: 2 additions & 1 deletion lib/rules/rule-empty-line-before/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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;
}
Expand Down
57 changes: 57 additions & 0 deletions lib/utils/__tests__/isFirstNested.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ describe("isFirstNested", () => {
@media (min-width: 0px) {
a { color: 'pink'; }
}
@media (min-width: 0px) { /* comment */
a { color: 'pink'; }
}
`);

root.walkRules(rule => {
Expand All @@ -29,18 +32,39 @@ describe("isFirstNested", () => {
a {
@include foo;
}
b { /* comment */
@include foo;
}
`);

root.walkAtRules(atRule => {
expect(isFirstNested(atRule)).toBe(true);
});
});

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 => {
Expand All @@ -54,6 +78,10 @@ describe("isFirstNested", () => {
a { color: 'pink'; }
b { color: 'pink'; }
}
@media (min-width: 0px) {
/* comment */
b { color: 'pink'; }
}
`);

root.walkRules("b", rule => {
Expand All @@ -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);
});
});
});
45 changes: 40 additions & 5 deletions lib/utils/isFirstNested.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's impossible for the code execution to reach here. One of the return statements in the for loop will always be executed. This is guaranteed because it returns if it finds the statement node in parent.nodes and that must happen because that node must be in its own parent's list of nodes.

If this is too strange I could change the return false; in the for loop into break; so then this return false; at the end of the function will be executed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nwoltman good, just question, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why comment starts with “istanbul ignore next:”?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest uses Istanbul for analyzing code coverage. That comment tells Istanbul not to count that line in the total code coverage since it's impossible to execute (the documentation is in the old Istanbul repo).

return false;
};