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

Add except: ["after-rule"] to rule-nested-empty-line-before #2188

Merged
merged 8 commits into from Dec 12, 2016

Conversation

3 participants
@rontav
Contributor

rontav commented Dec 10, 2016

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

#2179

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@jeddy3

@rontav Wow, that was quick. Thanks.

I think you might have slightly misunderstood the change to your original proposal, though. I've added some comments to this review which will hopefully explain things.

@@ -23,6 +23,11 @@ module.exports = function (opts) {
expectEmptyLineBefore = !expectEmptyLineBefore
}
// Optionally reverse the expectation for the first nested selector
if (optionsMatches(opts.options, "except", "after-rule") && opts.rule.prev().type === "decl") {

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

I think this should be opts.rule.prev().type === "rule".

The following test will likely fail if you use decl, and should hopefully highlight the shortcomings of the decl approach:

a {
  @media {
     b {}
  }
  
  d {}
  d {}
}

This is why I changed the option from "first-selected" to "after-rule" in the original issue.

@@ -23,6 +23,11 @@ module.exports = function (opts) {
expectEmptyLineBefore = !expectEmptyLineBefore
}
// Optionally reverse the expectation for the first nested selector

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

"Optionally reverse the expectation if a rule precedes this node"

@@ -152,6 +152,35 @@ The following patterns are *not* considered warnings:
}
```
### `except: ["after-rule"]`
Reverse the primary option if the rule is the first after a property.

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

"Reverse the primary option if the rule comes after rule"

Reverse the primary option if the rule is the first after a property.
For example, with `"never"`:

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

"For example, with "always":"

Once you switch to opts.rule.prev().type === "rule" you'll hopefully see why this should be "always" and not "never"

@@ -122,6 +122,40 @@ testRule(rule, {
testRule(rule, {
ruleName,
config: [ "always", { except: ["after-rule"] } ],

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

These tests will likely fail when you update the logic.

Can you also include a test (both passing and failing) for when a rule is preceded by an at-rule e.g.

a {
  @media {
    b {}
  }
  
  d {}
  d {}
}
@rontav

This comment has been minimized.

Contributor

rontav commented Dec 10, 2016

Also, I've added a test case for atrule.
At first, I didn't agree with the "backwards" logic, but I understand why is it more stable that way.

@jeddy3

@rontav Thanks for making the changes. It's looking great. I've just three more requests.

At first, I didn't agree with the "backwards" logic, but I understand why is it more stable that way.

I'm glad you understand now. The "after-*" options also scale well for more complex scenarios.

description: "media rule",
message: messages.expected,
}, {
code: "a {\r\n color: pink;\r\n b {\rcolor: red; \r}\r\n c {\rcolor: blue; \r}\r\n}",

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

There's three \r's here unaccompanied by \n's. Can you add the \n's please.

}, {
code: "a {\n color: pink;\n b {color: red; }\n c {color: blue; }\n}",
message: messages.expected,
}, {

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

Let's add a test here to show that empty lines before the second nested rule are being checked e.g.

{
code: "a {\n color: pink;\n\n b {color: red; }\n\n c {color: blue; }\n}",
message: messages.expected,
}
reject: [ {
code: "a {\n color: pink;\n\n b {color: red; } \n\n c {color: blue; }\n}",
message: messages.rejected,
}, {

This comment has been minimized.

@jeddy3

jeddy3 Dec 10, 2016

Member

Let's add a test here to show that empty lines before the second nested rule are being checked e.g.

{
code: "a {\n color: pink;\n b {color: red; } \n c {color: blue; }\n}",
message: messages.rejected,
}
@jeddy3

jeddy3 approved these changes Dec 10, 2016

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Dec 10, 2016

@rontav Excellent job. It LGTM. Another stylelint member will likely look it over before we merge though.

We'll squash the commits within github and update the changelog, so don't worry about doing that.

@davidtheclark

One nitpicky point in the docs, then let's merge.

@@ -152,6 +152,35 @@ The following patterns are *not* considered warnings:
}
```
### `except: ["after-rule"]`
Reverse the primary option if the rule comes after rule.

This comment has been minimized.

@davidtheclark

davidtheclark Dec 12, 2016

Contributor

Let's add "another": after another rule.

@jeddy3 jeddy3 merged commit 3029b2a into stylelint:master Dec 12, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Dec 12, 2016

@rontav Thanks again!

Changelog:

  • Added: except: ["after-rule"] option to rule-nested-empty-line-before (#2188).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment