Skip to content

Commit

Permalink
Fix false positives for shared-line comment as a first child (#4360)
Browse files Browse the repository at this point in the history 
There were false positives when beginning of the rule/at-rule was on a different
line than start of the block (`{`).
  • Loading branch information
@hudochenkov
hudochenkov committed Oct 17, 2019
1 parent 94c7a28 commit 58e4c24
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 51 deletions.
8 changes: 8 additions & 0 deletions lib/rules/comment-empty-line-before/__tests__/index.js
View file
Expand Up @@ -15,6 +15,14 @@ const alwaysTests = {
code: 'a { color: pink; /** comment */\ntop: 0; }',
description: 'shared-line comment ignored',
},
{
code: 'a { /** shared-line comment */\n color: pink; }',
description: 'shared-line comment ignored',
},
{
code: 'a,\n b { /** shared-line comment */\n color: pink; }',
description: 'shared-line comment ignored',
},
{
code: 'a {} /** comment */',
description: 'shared-line comment ignored',
Expand Down
150 changes: 100 additions & 50 deletions lib/utils/__tests__/isSharedLineComment.test.js
View file
Expand Up @@ -7,71 +7,97 @@ const postcss = require('postcss');
describe('isSharedLineComment', () => {
it('returns false for the first node', () => {
const root = postcss.parse(`
/* comment */
`);
/* comment */
`);

expect(isSharedLineComment(root.nodes[0])).toBe(false);
});

it('returns false for a non-shared-line comment before a rule', () => {
const root = postcss.parse(`
/* comment */
a {}
`);
/* comment */
a {}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(false);
});

it('returns false for a non-shared-line comment after a rule', () => {
const root = postcss.parse(`
a {}
/* comment */
`);
a {}
/* comment */
`);

expect(isSharedLineComment(root.nodes[1])).toBe(false);
});

it('returns true for a shared-line comment before a rule', () => {
const root = postcss.parse(`
/* comment */ a {}
`);
/* comment */ a {}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(true);
});

it('returns true for a shared-line comment after a rule', () => {
const root = postcss.parse(`
a {} /* comment */
`);
a {} /* comment */
`);

expect(isSharedLineComment(root.nodes[1])).toBe(true);
});

it('returns false for a shared-line non-comment', () => {
const root = postcss.parse(`
a {} b {}
`);
a {} b {}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(false);
expect(isSharedLineComment(root.nodes[1])).toBe(false);
});

it('returns true when comment shares a line with the start of a rule block, before it', () => {
const root = postcss.parse(`
/* comment */ a {
color: pink;
}
`);
/* comment */ a {
color: pink;
}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(true);
});

it('returns true when comment shares a line with the start of a rule block with a multiline selector, before it', () => {
const root = postcss.parse(`
/* comment */ a,
b {
color: pink;
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
});
});

it('returns true when comment shares a line with the start of a rule block, after it', () => {
const root = postcss.parse(`
a { /* comment */
color: pink;
}
`);
a { /* comment */
color: pink;
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
});
});

it('returns true when comment shares a line with the start of a rule block with a multiline selector, after it', () => {
const root = postcss.parse(`
a,
b { /* comment */
color: pink;
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
Expand All @@ -80,20 +106,44 @@ describe('isSharedLineComment', () => {

it('returns true when comment shares a line with the start of an at-rule block, before it', () => {
const root = postcss.parse(`
/* comment */ @media {@media (min-width: 0px)
a { color: pink; }
}
`);
/* comment */ @media (min-width: 0px) {
a { color: pink; }
}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(true);
});

it('returns true when comment shares a line with the start of an at-rule block with a multiline selector, before it', () => {
const root = postcss.parse(`
/* comment */ @media
(min-width: 0px) {
a { color: pink; }
}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(true);
});

it('returns true when comment shares a line with the start of an at-rule block, after it', () => {
const root = postcss.parse(`
@media (min-width: 0px) { /* comment */
a { color: pink; }
}
`);
@media (min-width: 0px) { /* comment */
a { color: pink; }
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
});
});

it('returns true when comment shares a line with the start of an at-rule block with a multiline selector, after it', () => {
const root = postcss.parse(`
@media
(min-width: 0px) { /* comment */
a { color: pink; }
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
Expand All @@ -102,26 +152,26 @@ describe('isSharedLineComment', () => {

it('returns false when comment shares a line with only another comment', () => {
const root = postcss.parse(`
/* comment */ /* comment */
`);
/* comment */ /* comment */
`);

expect(isSharedLineComment(root.nodes[0])).toBe(false);
});

it('returns true when comment shares a line with another comment and a non-comment', () => {
const root = postcss.parse(`
/* comment */ /* comment */ a {}
`);
/* comment */ /* comment */ a {}
`);

expect(isSharedLineComment(root.nodes[0])).toBe(true);
});

it('returns true when comment shares a line with the end of a multi-line rule block, after it', () => {
const root = postcss.parse(`
a {
color: pink;
} /* comment */
`);
a {
color: pink;
} /* comment */
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
Expand All @@ -130,12 +180,12 @@ describe('isSharedLineComment', () => {

it('returns true when comment shares a line with the end of a multi-line property declaration, after it', () => {
const root = postcss.parse(`
a {
border-radius:
1em
0; /* comment */
}
`);
a {
border-radius:
1em
0; /* comment */
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
Expand All @@ -144,12 +194,12 @@ describe('isSharedLineComment', () => {

it('returns true when comment shares a line with the start of a multi-line property declaration, before it', () => {
const root = postcss.parse(`
a {
/* comment */ border-radius:
1em
0;
}
`);
a {
/* comment */ border-radius:
1em
0;
}
`);

root.walkComments((comment) => {
expect(isSharedLineComment(comment)).toBe(true);
Expand Down
4 changes: 3 additions & 1 deletion lib/utils/isSharedLineComment.js
View file
Expand Up @@ -28,10 +28,12 @@ module.exports = function isSharedLineComment(node /*: postcss$node*/) /*: boole

const parentNode = node.parent;

// It's a first child and located on the same line as block start
if (
parentNode !== undefined &&
parentNode.type !== 'root' &&
parentNode.source.start.line === node.source.start.line
parentNode.index(node) === 0 &&
!node.raws.before.includes('\n')
) {
return true;
}
Expand Down

0 comments on commit 58e4c24

Please sign in to comment.