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 autofix to selector-combinator-space-after #3446

Merged
merged 6 commits into from Jul 12, 2018

Conversation

3 participants
@ota-meshi
Member

ota-meshi commented Jul 7, 2018

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

selector-combinator-space-after in #2829

Is there anything in the PR that needs further explanation?

ex.

  • option: always

    code:

    a >.b {}

    fixed:

    a > .b {}
  • option: never

    code:

    a > .b {}

    fixed:

    a >.b {}
@jeddy3

jeddy3 requested changes Jul 10, 2018 edited

@ota-meshi Thanks, looking good.

I've made a few requests as per the conventions:

Ensure you use standard CSS syntax by default, and only swap parsers when testing a specific piece of non-standard syntax.

So, any nesting tests using direct nesting (with the nesting selector) belong in the main testRule block without any syntax: ..., and any nesting tests without the nesting selector belong in a syntax: scss testRule block.

There are a lot of tests in stylelint and these conventions help us keep the code (including the tests) maintainable.

@@ -14,6 +14,8 @@ The descendant combinator is *not* checked by this rule.
Also, `+` and `-` signs within `:nth-*()` arguments are not checked (e.g. `a:nth-child(2n+1)`).
The `--fix` option on the command line can automatically fix all of the problems reported by this rule.

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Minor update to make it like the other READMEs:

The `--fix` option on the [command line](../../../docs/user-guide/cli.md#autofixing-errors) can automatically fix all of the problems reported by this rule.
@@ -135,79 +136,136 @@ testRule(rule, {
{
code: "namespace|type#id > .foo {}",
description: "qualified id with namespace"
},

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

I'm confused by these tests are they aren't valid CSS. Do we need them?

This comment has been minimized.

@ota-meshi

ota-meshi Jul 11, 2018

Member

I didn't know it was invalid css...
I removed this test case.

line: 1,
column: 13
},
{

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Can you move this and the following tests into the syntax: scss testRule block at the bottom of the file as nesting without the nesting selector (&) isn't in the draft spec?

line: 1,
column: 14
},
{

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Can you move this and the following tests into the syntax: scss testRule block at the bottom of the file, please?

description: "two spaces after + combinator",
message: messages.expectedAfter("+")
}
]
});
testRule(rule, {

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Empty line above, please.

config: ["always"],
fix: true,
accept: [

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Can you move the tests in this block into the first testRule block (the one without any syntax: property), please?

The syntax is valid draft spec so belongs in the first testRule.

@@ -143,6 +143,18 @@ testRule(rule, {
{
code: "namespace|type#id > .foo {}, space|customtype#id_withunder > a {}",
description: "qualified id with namespace selector list"
},

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Can you move this tests into their own testRule with syntax: scss as they are without a nesting selector?

@@ -209,6 +221,13 @@ testRule(rule, {
message: messages.expectedBefore(">>>"),
line: 1,
column: 2
},
{

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Move to testRule with syntax: scss.

@@ -313,6 +332,18 @@ testRule(rule, {
{
code: "namespace|type#id> .foo {}",
description: "qualified id with namespace"
},
{

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Move to testRule with syntax: scss.

@@ -400,6 +431,13 @@ testRule(rule, {
message: messages.rejectedBefore(">>>"),
line: 1,
column: 3
},
{

This comment has been minimized.

@jeddy3

jeddy3 Jul 10, 2018

Member

Move to testRule with syntax: scss.

@jeddy3 jeddy3 changed the title from Add autofix to `selector-combinator-space-after` to Add autofix to selector-combinator-space-after Jul 10, 2018

Fixed the following.
* Moved the testcases according to the syntax.
* Removed the invalid css testcases.
@ota-meshi

This comment has been minimized.

Member

ota-meshi commented Jul 11, 2018

@jeddy3 Thank you for reviews and many comments!
I changed it, so please check it again.

@jeddy3

jeddy3 approved these changes Jul 11, 2018

@ota-meshi Thanks so much for making the changes. Grouping the tests by syntax makes them much easier to manage and read.

LGTM

@jeddy3 jeddy3 merged commit 840d493 into stylelint:master Jul 12, 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 increased (+0.02%) to 96.014%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jul 12, 2018

  • Added: selector-combinator-space-after autofix (#3446).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment