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.

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 false positives for trailing combinator in selector-combinator-space-after #4878

Conversation

@malsf21
Copy link
Member

@malsf21 malsf21 commented Jul 23, 2020

Hey there!

This is a PR that tries to fix the false positive brought up in #4584, namely that a trailing combinator (such as the general sibling combinator, ~) is not properly handled by selector-combinator-space-after.

After some discussion in #4849, this PR implements a fix by updating the isStandardSyntaxCombinator rule util to ignore certain combinators that are the first or last combinator in their container. Since combinators should generally be between two different selectors, we can rule out combinators that... aren't. Then, we use this new utility in selectorCombinatorSpaceChecker, which then fixes the false positive in selector-combinator-space-after.

I also added some test to verify our functionality, for isStandardSyntaxCombinator and for the case mentioned in #4584 in selector-combinator-space-after.

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

Closes #4584.

Is there anything in the PR that needs further explanation?

First, a quick question about the > nesting selector: I had to ignore this, because the following SCSS is standard:

a {
  > b {
     ...
  }
}

Not sure if this is the right approach, or if I should instead update the existing tests. Any thoughts on this would be appreciated.

Also, a meta-note: this is a fresh version of the PR #4849, which had some issues after merging master back into the forked repository. Copied over the code to a fresh branch from master.

…paceChecker

copied over from a (now mangled) PR on malsf21/stylelint
@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Aug 1, 2020

Thank you for continuing to work on this issue. The pull request is almost ready, I think.

First, a quick question about the > nesting selector: I had to ignore this, because the following SCSS is standard:

The isStandard* utilities are for whitelisting standard CSS code. They're used to shield the built-in rules from edge cases introduced in the various CSS Processors like SCSS.

The > in:

a {
  > b {
     ...
  }
}

Is not, I believe, a standard CSS combinator.

or if I should instead update the existing tests.

Yes, I think so.

@malsf21
Copy link
Member Author

@malsf21 malsf21 commented Aug 4, 2020

Hey @jeddy3,

Thank you for taking the time to go back-and-forth with me on this PR.

I've removed the special case, and adjusted the test case in a way that I think best implements the feature - but, the caveat is that it actually removes some extra rejection cases. For example, consider the following for selector-combinator-space-after

a { 
  > /*comment*/a {
     
  } 
}

in conjunction with the test config

{
  code: 'a { >/*comment*/a {} }',
  fixed: 'a { > /*comment*/a {} }',
  description: 'scss nesting and comment',	
  message: messages.expectedAfter('>'),	
  line: 1,	
  column: 5,	
},

Previously, we would reject this. However, the new implementation of isStandardSyntaxCombinator treats the > as a non-standard syntax combinator (as you've indicated above), and thus skips this combinator/selector pair entirely, which leads to not rejecting the rule. In my opinion, this seems quite reasonable, since this is the same philosophy behind the problem in the original issue #4584; however, I do want to clarify if I'm taking this approach properly.

Another way I could re-do these specific test cases (and still keep the original test ) is to add the & inherit selector before the >, like so:

{
  code: 'a { & >/*comment*/a {} }',
  fixed: 'a { & > /*comment*/a {} }',
  description: 'scss nesting and comment',	
  message: messages.expectedAfter('>'),	
  line: 1,	
  column: 7,	
},

But this kind of dodges the main problem, which is dropping certain rejection cases, and I don't think it actually tests for scss nesting (since the inside is just like any normal selector).

Sorry for this wall of text, but I want to make sure I get this right! Let me know what you think, and if there's a different option I should take. If not, I think the PR is good to be reviewed!

@jeddy3
jeddy3 approved these changes Aug 9, 2020
Copy link
Member

@jeddy3 jeddy3 left a comment

Thank you, LGTM!

In my opinion, this seems quite reasonable, since this is the same philosophy behind the problem in the original issue #4584; however, I do want to clarify if I'm taking this approach properly.

Yes, you are. The built-in rules are geared towards standard CSS. A plugin author can, if they like, create scss/selector-combinator-space-after that deals with non-standard things like the implicit nesting selector. This is already the case for many other SCSS and SCSS-like syntax.

@jeddy3 jeddy3 mentioned this pull request Aug 9, 2020
6 of 6 tasks complete
@jeddy3 jeddy3 merged commit 2c4d77f into master Aug 28, 2020
11 checks passed
11 checks passed
CodeQL
Details
CodeQL
Details
Lint on Node.js 12 and ubuntu-latest
Details
Test on Node.js 10 and ubuntu-latest
Details
Test on Node.js 10 and windows-latest
Details
Test on Node.js 10 and macos-latest
Details
Test on Node.js 12 and windows-latest
Details
Test on Node.js 12 and macos-latest
Details
Coverage on Node.js 12 and ubuntu-latest
Details
CodeQL No new alerts
Details
coverage/coveralls Coverage decreased (-0.01%) to 96.532%
Details
@jeddy3 jeddy3 deleted the fix/selector-combinator-space-after-trailing-combinator-false-positive branch Aug 28, 2020
m-allanson added a commit that referenced this pull request Sep 3, 2020
* master: (34 commits)
  Update CHANGELOG.md
  Fix double-slash disable comments when followed by another comment (#4913)
  Update CHANGELOG.md (#4916)
  13.7.0
  Prepare 13.7.0
  Prepare changelog
  Update dependencies
  Update CHANGELOG.md
  Deprecate *-blacklist/*-requirelist/*-whitelist (#4892)
  Fix some path / glob problems (#4867)
  Update CHANGELOG.md
  Add a reportDescriptionlessDisables flag (#4907)
  Fix CHANGELOG.md format via Prettier (#4910)
  Fix callbacks in tests (#4903)
  Update CHANGELOG.md
  Fix false positives for trailing combinator in selector-combinator-space-after (#4878)
  Add coc-stylelint (#4901)
  Update CHANGELOG.md
  Add support for *.cjs config files (#4905)
  Add a reportDisables secondary option (#4897)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.