Fix how "except" options are checked in *-empty-line-before rules #2920
Conversation
|
@nwoltman Oh wow, thanks for the quick fix!
It's surprising that this hasn't come to light before! Good catch, though. Is there still an issue here with shared-line comments not being ignored by @media screen { /* comment */
.foo {
display: none;
}
}The I think we should address this within this PR. What do you think? |
| } | ||
|
|
||
| function isAfterSingleLineComment(rule) { | ||
| const prevRule = rule.prev(); |
There was a problem hiding this comment.
Should this use the isSharedLineComment util?
There was a problem hiding this comment.
Turns out there's already an isAfterCommentLine util so this function doesn't even need to exist.
There was a problem hiding this comment.
Whoops, never mind about that; isAfterCommentLine also counts multi-line comments.
Also, when I fix this function (by using isSharedLineComment), a few tests start failing:
rule-empty-line-before › accept › ["always", {"except": ["after-single-line-comment"]}]
@media { /* comment */
a {}
}rule-empty-line-before › reject › ["always", {"except": ["after-single-line-comment"]}]
@media { /* comment */
a {}
}They actually need to be reversed now because the shared-line comment doesn't count as a single-line comment, so the first test should be rejected and the second one should be accepted. This might need to be highlighted in the release notes of the release that includes this fix.
There was a problem hiding this comment.
They actually need to be reversed now because the shared-line comment doesn't count as a single-line comment
Sounds about right. I had assumed we weren't testing for this eventuality. However, it turns out we are but the tests are wrong.
This might need to be highlighted in the release notes of the release that includes this fix.
Good point. FYI, due to our Semantic Versioning Policy, this can be included in a minor release, even though it could break some users' builds.
There was a problem hiding this comment.
Whoops, never mind about that;
isAfterCommentLinealso counts multi-line comments.
Feel free to create a isAfterSingleLineComment util, though.
And also free feel to rename the isAfterCommentLine util to isAfterComment for consistency with the other isAfter* utils. I'm not sure the Line suffix makes sense now that shared-line comments are always ignored.
| @@ -153,6 +131,20 @@ const rule = function(expectation, options, context) { | |||
| }; | |||
| }; | |||
|
|
|||
| function isAfterRule(rule) { | |||
| const prevRule = rule.prev(); | |||
There was a problem hiding this comment.
Should this use the getPreviousNonSharedLineCommentNode util? Like custom-property-empty-line-before does? If so, can we get a test for this.
|
@jeddy3 Ah yes, you're right that shared-line comments should be ignored and shouldn't trigger |
|
@jeddy3 I've updated the PR. Would it be alright if I renamed the |
| { | ||
| code: | ||
| "@media screen { /* comment */\n .foo {\n display: none;\n }\n}", | ||
| description: "shared-line comment - issue #2919" |
There was a problem hiding this comment.
We should try to do this more often when warranted 👌
Fixes #2919
The bug reported in #2919 was being caused by multiple "except" options being checked individually, which caused the expectation to flip-flop and be incorrect if an even number of exceptions were satisfied.
This PR fixes that type of code in the various
*-empty-line-beforerules so that as soon as one exception is satisfied, it skips the rest. The other*-empty-line-beforerules that I did not touch were already doing this correctly.