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 false positives for shared-line comment as a first child #4360

Merged
merged 1 commit into from Oct 17, 2019
Merged

Fix false positives for shared-line comment as a first child #4360

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
8 changes: 8 additions & 0 deletions lib/rules/comment-empty-line-before/__tests__/index.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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