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 autofixing for *-empty-line-before rules #2500

Merged
merged 6 commits into from Apr 24, 2017

Conversation

3 participants
@hudochenkov
Member

hudochenkov commented Apr 13, 2017

  • at-rule-empty-line-before
  • custom-property-empty-line-before
  • declaration-empty-line-before
  • rule-empty-line-before

comment-empty-line-before done in initial autofix PR #2467.

I didn't do block-closing-brace-empty-line-before, because it's quite different from rules above. Also it's not possible to use addEmptyLineBefore and removeEmptyLinesBefore utils, because they change node.raws.before, but for this rule node.raws.after should be changed.

I'm not sure why result returns in comment-empty-line-before. I've tried return and return result, tests were passing every time. I decided to use just return.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 13, 2017

Couldn't be merged until #2501 is fixed. Labeling as status: blocked.

@hudochenkov hudochenkov changed the title from [WIP] Add autofixing for *-empty-line-before rules to Add autofixing for *-empty-line-before rules Apr 15, 2017

@hudochenkov hudochenkov force-pushed the empty-line-before-autofix branch from a93ead5 to 3d4c0a6 Apr 22, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 22, 2017

Thanks to @davidtheclark tests are working consistently regardless of the platform now! This PR is ready for a review.

I thinks these rules are enough for the new version to be released.

@@ -12,6 +12,8 @@ a {}
If the at-rule is the very first node in a stylesheet then it is ignored.
The `--fix` option on the [command line](../../../docs/user-guide/cli.md#autofixing-errors) can automatically fix some of the problems reported by this rule.

This comment has been minimized.

@davidtheclark

davidtheclark Apr 22, 2017

Contributor

Can it only fix "some"? Not "all"?

@@ -42,33 +42,43 @@ const sharedAlwaysTests = {
reject: [ {
code: "a {} @media {}",
fixed: "a {}\n\n @media {}",

This comment has been minimized.

@davidtheclark

davidtheclark Apr 22, 2017

Contributor

That extra space before @ seems undesireable.

This comment has been minimized.

@hudochenkov

hudochenkov Apr 23, 2017

Member

What if we use similar code?

div {\n  a {}  @media {}\n}

Which is looks like:

div {
  a {}  @media {}
}

We have have two spaces before a and @media. In this example if we add \n\n before two spaces we get correct indentation. These two examples basically the same. There is some amount of spaces before node we're fixing, but we can't know if they are good of bad.

message: messages.rejected,
}, {
code: "a {\n\n color: pink;\n\n @mixin foo;\n}",
fixed: "a {\n\n color: pink;\n @mixin foo;\n}",

This comment has been minimized.

@davidtheclark

davidtheclark Apr 22, 2017

Contributor

Here the spaces before @ make sense. I think we need to distinguish these cases. If there was a space after a newline — i.e. an indentation — preserve that; but do not preserve the space when replace a space with newlines.

This comment has been minimized.

@hudochenkov

hudochenkov Apr 23, 2017

Member

We can't know if it's indentation or not without interfering with other rules.

@if(true) {
}
@else-mixin {

This comment has been minimized.

@davidtheclark

davidtheclark Apr 22, 2017

Contributor

This looks like bad indentation.

This comment has been minimized.

@hudochenkov

hudochenkov Apr 23, 2017

Member

This rule is not about indentation.

@@ -32,46 +33,49 @@ testRule(rule, {
reject: [ {
code: "a { top: 15px; }",
fixed: "a {\n\n top: 15px; }",

This comment has been minimized.

@davidtheclark

davidtheclark Apr 22, 2017

Contributor

Problem here is that we're going to have to decide how to indent or not indent. I think keeping the space is not right, though: that space should be replaced by the newlines, not added to.

This comment has been minimized.

@hudochenkov

hudochenkov Apr 23, 2017

Member

We have rules designed to do only their thing. We don't have overlapping rules. So why *-empty-line-before should care about anything except empty lines before? :) We have indentation rule to take care of indentation.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 23, 2017

I think it's not enough to run stylelint only once to autofix errors.

Let's imagine we have autofixing for both rules:

{
    "rules": {
        "indentation": 4,
        "rule-empty-line-before": "always"
    }
}

This is input:

a { b {} }

This is result (1 space before b):

a {\n\n b {} }

What is expected (4 spaces before b):

a {\n\n    b {} }

indentation runs first, it found no problems in input code, so it does nothing. Then runs rule-empty-line-before and add empty line before. And stylelint is done. But if we run stylelint without --fix flag after that we'll get new error from indentation. So we should run with --fix two times to eliminate all arrors.

For the first experimental release we're doing fine, in my opinion. But for a release after that we should address this problem. Something like that: with --fix run stylelint at least two times. If there errors from rules, which have autofixing run stylelint again. If errors are fixed in the last run, make another run, maybe new errors were introduced.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 23, 2017

@hudochenkov: Your solution relies on somebody using the indentation rule. Maybe they aren't. If I weren't (because maybe I have wacky indentation preferences), it would be a real bummer to have my code automatically formatted in a way that looked terrible — that would be less helpful than no automatic formatting, because I'd have to go back and fix the fixing manually.

I agree that our rules each do their own thing, but I don't think that's an excuse to make an automatic fix create code that looks worse rather than better. If we can't make the code come out better after fixing, I don't think we should make the change.

I also agree that multiple runs might be necessary.

I wonder if this problem will crop up mostly with indentation. How could we avoid it? Here's a quick brainstorm (any other ideas?):

  • Enforce that certain rules can only be autofixed if indentation is set in the config. Then we can use the indentation value when fixing.
  • When indentation is not set, infer an indentation style based on the rest of the file, and use that.
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 23, 2017

I think cases like a {} @media {} are edge cases. In a real code base these rules most likely are on separate lines.

Also I believe the most of our users have indent rule enabled.

Yes, it's just assumptions. But I think they are very close to reality, especially the first one.

I propose don't overengineer this now. Autofixing is an experimental feature. Let's release it in a current state, because for the most use cases it will work great. And we'll see how users react to this feature and what we can improve.

More practical proposal. Let's add autofixing for indentation for the first release also. And since we can't run autofixing multiple times yet, we can put indentation at the very end of rules/index.js so it will run after every *-empty-line-before rule and fix possible inconsistency. We may add a note to *-empty-line-before docs, that we recommend to enable indentation rule if user want to use autofixing.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 24, 2017

we'll see how users react to this feature and what we can improve

If we already know what needs to be fixed, I think we should fix that before releasing it.

Let's add autofixing for indentation for the first release also ... we can put indentation at the very end ... We may add a note to *-empty-line-before docs, that we recommend to enable indentation rule if user want to use autofixing.

That's exactly the plan I had in mind 👍

I'll approve this PR if you want to merge this.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 24, 2017

we can put indentation at the very end ...

Sounds like a good idea to me. It'll help us mitigate, at least in the short term, having to do waves of fixes.

Great job with this PR btw! 😄

@jeddy3

jeddy3 approved these changes Apr 24, 2017

@hudochenkov hudochenkov merged commit 489c8f0 into master Apr 24, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 95.467%
Details

@hudochenkov hudochenkov deleted the empty-line-before-autofix branch Apr 24, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 24, 2017

Updated changelog to look like:

  • Added: experimental autofixing (#2467). Use --fix CLI parameter or fix: true Node API options property. Supported rules:
    • at-rule-empty-line-before
    • at-rule-name-case
    • comment-empty-line-before
    • custom-property-empty-line-before
    • declaration-empty-line-before
    • rule-empty-line-before

@jeddy3 jeddy3 referenced this pull request May 14, 2017

Closed

Release 7.11 #2573

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment