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 selector-combinator-*list rules #3088

Merged
merged 3 commits into from Jan 6, 2018

Conversation

4 participants
@jeddy3
Member

jeddy3 commented Dec 29, 2017

Closes #3013

@hudochenkov

Good job!

Some things are controversial.

});
});
it("descendant (double child)", () => {
return rules("a >> b {}", func => {

This comment has been minimized.

@hudochenkov

hudochenkov Dec 29, 2017

Member

I can't find it in a specification. Doesn't look like a standard one.

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

It's a level 4 one. I'll take it out though

```
```css
a >>> b {}

This comment has been minimized.

@hudochenkov

hudochenkov Dec 29, 2017

Member

It's not a standard combinator. Maybe it's better not to add it to readme?

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

Good point

return (root, result) => {
const validOptions = validateOptions(result, ruleName, {
actual: blacklist,
possible: [_.isString]

This comment has been minimized.

@hudochenkov

hudochenkov Dec 29, 2017

Member

It should also check for _.isRegExp according to readme. It would be great to have RegExp in tests as well. Both as a string and as RegExp.

The same comment applies to the second rule.

## Options
`array|string|regex`: `["array", "of", "unprefixed", "pseudo-elements" or "regex"]|"pseudo-element"|/regex/`

This comment has been minimized.

@hudochenkov

hudochenkov Dec 29, 2017

Member

These rule about combinators.

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

Oops

## Options
`array|string|regex`: `["array", "of", "unprefixed", "pseudo-elements" or "regex"]|"pseudo-element"|/regex/`

This comment has been minimized.

@hudochenkov

hudochenkov Dec 29, 2017

Member

These rule about combinators.

```
```css
a >>> b {}

This comment has been minimized.

@hudochenkov

hudochenkov Dec 29, 2017

Member

It's not a standard combinator. Maybe it's better not to add it to readme?

@CAYdenberg

General points:

Can we add a couple tests for combinators without spaces and where " " is not specified as a combinator?

I'm thinking that POSSIBLY -whitelist should assume " " is a valid combinator and add it automatically to the config? As a user, it's so common that I'm not even sure I think about it. Could be a source of confusion resulting in bug reports we have to sift through later ... (this may go against the core values of stylelint and I'm good on it either way, just wanted to throw it out there ...)

I'm going to push back a bit on allowing RegExp's ... most combinators are just a single special character (+, ~) and I don't see this feature being all that useful. Puts cognitive load on the user and maintainers for possibly not much gain. I could be convinved otherwise, I just don't see a real use for it.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 1, 2018

I'm going to push back a bit on allowing RegExp's ... most combinators are just a single special character (+, ~) and I don't see this feature being all that useful.

That's a fair point. I originally intended to add RegExps for controlling what whitespace is allowed for descendant combinators. I've updated the tests and examples to show this, rather than use the child and piercing combinators.

I'm thinking that POSSIBLY -whitelist should assume " " is a valid combinator and add it automatically to the config?

I think there is a use case for not allowing the descendant combinator e.g. a completely flat specificity graph or when (exclusively) using CSS Modules' composes system.

Having said all that, perhaps it is better to split this up:

  • selector-descendant-combinator-no-non-space (already built)
  • selector-no-descendent-combinator
  • selector-combinator-blacklist
  • selector-combinator-whitelist

The latter two would ignore whitespace descendant combinators and not accept RegExp.

What do you think?

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Jan 1, 2018

I think there is a use case for not allowing the descendant combinator e.g. a completely flat specificity graph or when (exclusively) using CSS Modules' composes system.

Fair. I'm sold on this the way it is. Having to create another rule and to handle an exception to this one seems like a waste. Maybe just make sure the README specifically mentions that " " is itself a combinator and needs to be whitelisted explicitly.

I originally intended to add RegExps for controlling what whitespace is allowed for descendant combinators.

Yeah, this seems like a different concern to me. Stylistic rather than functional.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 1, 2018

Ummm... I think you've (despite your conclusion) talked me out of not breaking the rule up.

Having to create another rule and to handle an exception to this one seems like a waste.

This rule is unique amongst the -*list rules because the decedent combinator itself isn't a simple string comparison. So, I think there's a case for splitting selector-no-descendent-combinator out and keeping the -*list rules consistent.

Yeah, this seems like a different concern to me. Stylistic rather than functional.

That's a good point. Keeping support for the descendant combinator in makes this rule half functional and half stylistic.

@ntwb ntwb referenced this pull request Jan 3, 2018

Closed

Release 9.0.0 #3099

6 of 6 tasks complete
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 3, 2018

I agree with @CAYdenberg. RegExp here might be redundant.

Yeah, this seems like a different concern to me. Stylistic rather than functional.

That's a good point. Keeping support for the descendant combinator in makes this rule half functional and half stylistic.

I believe these rules should be functional and not about the style of code.

To avoid using RegExp and avoid stylistic concerns we could replace /\s+/ in selectors with single space and do comparisons against normalized selector.

## Options
`array|string|regex`: `["array", "of", "unprefixed", "combinators" or "regex"]|"combinator"|/regex/`

This comment has been minimized.

@hudochenkov

hudochenkov Jan 3, 2018

Member

What are prefixed combinators? :)

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 4, 2018

To avoid using RegExp and avoid stylistic concerns we could replace /\s+/ in selectors with single space and do comparisons against normalized selector.

That's a great idea. It does away with the need for the selector-no-descendent-combinator rule and can be easily communicated in the README.

I'll make the change now.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 4, 2018

@hudochenkov & @CAYdenberg This is ready for review again.

@hudochenkov

Good job, @jeddy3!

Good catch on rule complexity and scope, @CAYdenberg!

@CAYdenberg CAYdenberg merged commit 66f0ad6 into master Jan 6, 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 Coverage decreased (-0.008%) to 95.648%
Details

@CAYdenberg CAYdenberg deleted the issue-3013 branch Jan 6, 2018

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 6, 2018

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Jan 6, 2018

I did ... just took a few extra minutes because I also have kids 🤕

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 6, 2018

@CAYdenberg sorry, man. Saw you started answering other issues, and though you forgot. Sorry.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Jan 7, 2018

NP ... appreciate the respect for our process.

@ntwb

This comment has been minimized.

Member

ntwb commented Jan 8, 2018

Changelog: 741cd6e#diff-4ac32a78649ca5bdd8e0ba38b7006a1e

  • Added: selector-combinator-*list rules (#3088).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment