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.

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

Fix bug with quasi-qualified selector in block-opening-brace-newline-after #3383

Merged
merged 5 commits into from Jun 23, 2018

Conversation

3 participants
@YozhikM
Contributor

YozhikM commented Jun 9, 2018

Which issue, if any, is this issue related to?

#3366 Bug fix

Is there anything in the PR that needs further explanation?

No

@YozhikM YozhikM requested review from hudochenkov, jeddy3 and evilebottnawi Jun 9, 2018

@YozhikM YozhikM changed the title from Fix bug with quasi-qualified selector in `block-opening-brace-newline-after` to Fix bug with quasi-qualified selector in block-opening-brace-newline-after Jun 12, 2018

@hudochenkov

Thank you for a fix, @YozhikM!

I think current rule implementation ignores comments to allow following cases:

@media print { /*.test2*/
    .test3 {
        color: #f00;
    }
}

For some people this fix will break their builds, because of new behaviour change. I'd release this fix in major version.

P. S. @YozhikM, don't need to assign people to PRs. It feels like we have to review, which shouldn't be true in 99% cases. PRs are reviewed by contributors who have time to do this. Open source is not our job, it's our hobby :)

},
{
code: ".a {\r\n/*.b*/.c {\r\n color: pink; }\r\n }",
description: "next-line comment and CRLF with quasi-qualified selector"

This comment has been minimized.

@hudochenkov

hudochenkov Jun 17, 2018

Member

Could you add the following examples as accepted, please?

@media print {\n /*.test2*/.a {\n color: pink;\n }\n }
@media print {\r\n /*.test2*/.a {\r\n color: pink;\r\n }\r\n }
@media print {\n /*.test2*/\n .a {\n color: pink;\n }\n }
@media print {\r\n /*.test2*/\r\n .a {\r\n color: pink;\r\n }\r\n }
description: "next node is quasi-qualified selector without newline after",
message: messages.expectedAfter(),
line: 1,
column: 5

This comment has been minimized.

@hudochenkov

hudochenkov Jun 17, 2018

Member

Could you add the following examples as rejected, please?

.a {/*.b*/\n.c { color: pink; } }
.a {/*.b*/\r\n.c { color: pink; } }
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 17, 2018

For some people this fix will break their builds, because of new behaviour change. I'd release this fix in major version.

On the second thought, accepted tests have a { /* 1 */\n color: pink;\n}. So it might be a non-breaking change. @YozhikM Could you verify, please?

@YozhikM

This comment has been minimized.

Contributor

YozhikM commented Jun 17, 2018

@hudochenkov Sure.

@YozhikM YozhikM removed the request for review from jeddy3 Jun 17, 2018

@hudochenkov

Thank you for adding tests.

There is one concern about code reliability.

if (!startNode || !startNode.next) return null;
if (startNode.type === "comment") {
const newLines = ["\n", "\r\n", "\n ", "\r\n "];

This comment has been minimized.

@hudochenkov

hudochenkov Jun 17, 2018

Member

Something wrong about this. Maybe use regexp instead of an array and indexOf:

const reNewLine = /\r?\n/;

const index = reNewLine.test(startNode.raws.before);
if (!startNode || !startNode.next) return null;
if (startNode.type === "comment") {
const newLines = ["\n", "\r\n", "\n "];

This comment has been minimized.

@hudochenkov

hudochenkov Jun 18, 2018

Member

If I understand correctly, then \n\t or non-breakable space (unicode representation of  ) will be false positive with these conditions?

This comment has been minimized.

@YozhikM

YozhikM Jun 23, 2018

Contributor

@hudochenkov Hi, I've been busy all week. Changed the check, check my changes, please

@hudochenkov

Thank you!

@hudochenkov hudochenkov merged commit 64dbbcf into stylelint:master Jun 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 96.001%
Details

@YozhikM YozhikM deleted the YozhikM:issue-3366 branch Jun 23, 2018

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 23, 2018

  • Fixed: block-opening-brace-newline-after false positives for nested rule-sets prefixed with comments (#3383).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment