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

Add autofix to declaration-block-trailing-semicolon #3382

Conversation

YozhikM
Copy link
Contributor

@YozhikM YozhikM commented Jun 9, 2018

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

#3369 Add auto fix.

Is there anything in the PR that needs further explanation?

No.

/cc @evilebottnawi

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YozhikM Thanks for starting this.

I've made some requests.

Additionally, can you add the following to the rule README, please?:

The --fix option on the command line can automatically fix all of the problems reported by this rule.

Like so.

@@ -145,3 +145,35 @@ testRule(rule, {
}
]
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// auto-fix
if (context.fix) {
node.parent.raws.semicolon = ";";
Copy link
Member

@jeddy3 jeddy3 Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the raws.semicolon property expects a boolean.

You need to account for the "never" option too i.e. the trailing semicolon is removed when fixed (as this is a valid choice). This will be revealed when you add fix: true to the two existing rejects.

@jeddy3 jeddy3 changed the title Add autofix to declaration-block-trailing-semicolon Add autofix to declaration-block-trailing-semicolon Jun 10, 2018
- Add fix describing to README;
- Change tests, add fixed to rejected;
- Fix semicolon position for atrule after autofix.
@YozhikM
Copy link
Contributor Author

YozhikM commented Jun 11, 2018

@jeddy3 I fixed it

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YozhikM Thanks for making the changes. I think it's almost there. I've made a minor request and asked for an explanation for one bit of the code.

@@ -16,6 +16,8 @@ This rule ignores:
- trailing `//` comments
- declaration blocks containing nested (at-)rules

The --fix option on the command line can automatically fix all of the problems reported by this rule.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please backtick the flag e.g.

The `--fix` option on the command line can automatically fix all of the problems reported by this rule.

@YozhikM
Copy link
Contributor Author

YozhikM commented Jun 12, 2018

Done.

// auto-fix
if (context.fix) {
node.parent.raws.semicolon = true;
if (node.type === "atrule") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this code isn't clear. What does it do?

Copy link
Contributor Author

@YozhikM YozhikM Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For such cases as @import. To make it clearer, I will change it to node.type === "atrule" && node.name === "import". Because it was the only case, for which this check was needed.

@jeddy3
Copy link
Member

jeddy3 commented Jun 12, 2018

Because it was the only case, for which this check was needed.

Thanks for the explanation.

I think you've uncovered a weakness in how the test file is structured. This rule was one of the first we wrote and the structure of the test file does not adhere to current conventions. I suggest we restructure the test file in this PR to makes things clearer.

Can you use the following test file, please?

"use strict";

const rule = require("..");
const { messages, ruleName } = rule;

testRule(rule, {
  ruleName,
  config: ["always"],
  fix: true,

  accept: [
    {
      code: "a { color: pink; }",
      description: "single declaration block with trailing semicolon"
    },
    {
      code: "a { background: orange; color: pink; }",
      description: "multi declaration block with trailing semicolon"
    },
    {
      code: "a { &:hover { color: pink; }}",
      description: "nesting without first-level decl"
    },
    {
      code: "a { color: red; &:hover { color: pink; }}",
      description: "nesting with first-level decl"
    }
  ],

  reject: [
    {
      code: "a { color: pink }",
      fixed: "a { color: pink; }",
      description: "single declaration block without trailing semicolon",
      message: messages.expected,
      line: 1,
      column: 15
    },
    {
      code: "a { background: orange; color: pink }",
      fixed: "a { background: orange; color: pink; }",
      description: "multi declaration block without trailing semicolon",
      message: messages.expected,
      line: 1,
      column: 35
    },
    {
      code: "a { &:hover { color: pink }}",
      fixed: "a { &:hover { color: pink; }}",
      description: "nesting without first-level decl",
      message: messages.expected,
      line: 1,
      column: 26
    },
    {
      code: "a { color: red; &:hover { color: pink }}",
      fixed: "a { color: red; &:hover { color: pink; }}",
      description: "nesting with first-level decl",
      message: messages.expected,
      line: 1,
      column: 39
    }
  ]
});

testRule(rule, {
  ruleName,
  config: ["never"],
  fix: true,

  accept: [
    {
      code: "a { color: pink }",
      description: "single-line declaration block without trailing semicolon"
    },
    {
      code: "a { background: orange; color: pink }",
      description: "multi-line declaration block without trailing semicolon"
    }
  ],

  reject: [
    {
      code: "a { color: pink; }",
      fixed: "a { color: pink }",
      description: "single-line declaration block with trailing semicolon",
      message: messages.rejected,
      line: 1,
      column: 15
    },
    {
      code: "a { background: orange; color: pink; }",
      fixed: "a { background: orange; color: pink }",
      description: "multi-line declaration block with trailing semicolon",
      message: messages.rejected,
      line: 1,
      column: 35
    }
  ]
});

testRule(rule, {
  ruleName,
  config: ["always"],
  syntax: "scss",
  fix: true,

  accept: [
    {
      code: "a { @includes foo; }",
      description: "at-rule with trailing semicolon"
    },
    {
      code: "a { @foo { color: pink; } }",
      description: "at-rule with decl block with trailing semicolon"
    }
  ],

  reject: [
    {
      code: "a { @includes foo }",
      fixed: "a { @includes foo; }",
      description: "at-rule without trailing semicolon",
      message: messages.expected,
      line: 1,
      column: 15
    },
    {
      code: "a { @foo { color: pink } }",
      fixed: "a { @foo { color: pink; } }",
      description: "at-rule with decl block without trailing semicolon",
      message: messages.expected,
      line: 1,
      column: 22
    }
  ]
});

testRule(rule, {
  ruleName,
  config: ["never"],
  syntax: "scss",
  fix: true,

  accept: [
    {
      code: "a { @includes foo }",
      description: "at-rule without trailing semicolon"
    },
    {
      code: "a { @foo { color: pink } }",
      description: "at-rule with decl block without trailing semicolon"
    }
  ],

  reject: [
    {
      code: "a { @includes foo; }",
      fixed: "a { @includes foo }",
      description: "at-rule with trailing semicolon",
      message: messages.rejected,
      line: 1,
      column: 15
    },
    {
      code: "a { @foo { color: pink; } }",
      fixed: "a { @foo { color: pink } }",
      description: "at-rule with decl block with trailing semicolon",
      message: messages.rejected,
      line: 1,
      column: 22
    }
  ]
});

I've:

  • moved the non-standard nesting of blocked and blockless at-rules to separate testRules which use the syntax: "scss" option
  • changed the nested blockless at-rule to @includes to make it clear it's non-standard
  • removed the old-school double curly bracket nesting syntax

You might need to tweak some of the column values to get the tests to pass.

I think you'll also need to revert your recent "atrule" && node.name === "import" change too as this line is for non-standard nested blockless at-rules.

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YozhikM Thanks for making all the changes and for your patience. LGTM.

@hudochenkov hudochenkov merged commit 33386af into stylelint:master Jun 17, 2018
@hudochenkov
Copy link
Member

  • Added: declaration-block-trailing-semicolon autofix (#3382).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants