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

Indentation autofixing #2529

Merged
merged 4 commits into from Jun 9, 2017

Conversation

4 participants
@hudochenkov
Member

hudochenkov commented Apr 25, 2017

I've made autofixing for most use cases. These use cases was analyzed with PostCSS AST, so I could easily apply fixes. But for big number of other use cases this rule uses style-search. It's used for indentation inside declaration values, selectors, and at-rule parameters.

I haven't figured out how to deal with this yet. I've commented all reject cases which isn't covered by current autofixing.

Any suggestions how to deal with this problem?

Also there is a problem with Less syntax. I haven't investigate what causes the problem yet.

In order to finish this PR we need:

  • Finish autofixing for all other use cases
  • Add a note to rule's readme and rules list about autofixing
  • Add a note to *-empty-line-before rules with recommendation to enable indentation rule

P. S. It was total surprise to me, that this rule has secondary options 😯

Error log with Less mixins and autofixing

Semicolon gets removed.

 FAIL  lib/rules/indentation/__tests__/functions.js
  ● indentation › accept › Less mixin with multi-line arguments

    expect(received).toBe(expected)

    Expected value to be (using ===):
      ".foo {
      .mixin(
        @foo,
        @bar,
        @baz
      );
    }"
    Received:
      ".foo {
      .mixin(
        @foo,
        @bar,
        @baz
      )
    }"

    Difference:

    - Expected
    + Received

     .foo {
       .mixin(
         @foo,
         @bar,
         @baz
    -  );
    +  )
     }

      at stylelint.then.output (jest-setup.js:59:35)

  ● indentation › accept › Less mixin with multi-line arguments

    Error
      Error: expect(received).toBe(expected)

      Expected value to be (using ===):
        ".foo {
        .mixin(
          @foo,
          @bar,
          @baz
        );
      }"
      Received:
        ".foo {
        .mixin(
          @foo,
          @bar,
          @baz
        )
      }"

      Difference:

      - Expected
      + Received

       .foo {
         .mixin(
           @foo,
           @bar,
           @baz
      -  );
      +  )
       }
      at stylelint.then.output (jest-setup.js:59:35)

@hudochenkov hudochenkov changed the title from [WIP] Indentation fixing to [WIP] Indentation autofixing Apr 25, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 26, 2017

Looking good!

Also there is a problem with Less syntax. I haven't investigate what causes the problem yet.

I think in #2259 we decided that advanced features like autofixing would only be for standard-syntax. We don't want to repeat the path of stylefmt where the project stagnated due to the quantity of non-standard bugs for this advanced feature. I think we can remove any non-standard tests and make it clear within the documentation that autofix is a standard-syntax only feature. I'm still plugging away at a VISION.md doc, that we'll be able to point users towards if they want to know more background reasoning to this decision.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 26, 2017

@jeddy3 sounds reasonably, but most of our users use scss/sass, And it will be for them not too joyful news, I do not think that we should at the moment not support non-standard syntaxs

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 26, 2017

@evilebottnawi I believe, SCSS will work just fine, because parser is good. Less parser is a mess, and I have never used Less. So I'm not going to do anything for Less, but for SCSS I can do necessary changes, because I'm using it on my new job.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 26, 2017

@jeddy3 @hudochenkov i think we should fix only standard construction (isStandard*) and etc, rules not related stylelint can fix non standard constructions related to own syntax

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 26, 2017

i think we should fix only standard construction (isStandard*)

This feels like a sensible approach.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 26, 2017

@jeddy3 I believe that we should not touch anything what don't pass isStandard*, even if we can do it, otherwise it will be just a terrible collapse of errors and logic. But we can think about joining your autofixers for the rules, example:

"comment-empty-line-before": [ "always", {}, {fix: "path/fix.js"}],

if someone will want to use their fixes, I do not think that it will be difficult to implement

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 26, 2017

"comment-empty-line-before": [ "always", {}, {fix: "path/fix.js"}],

That's very interesting, and very much aligned with our philosophy of extensibility (plugins, processors, custom syntaxes, custom formatters and shareable configs).

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 26, 2017

/cc @davidtheclark what do you think about this idea (#2529 (comment))?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 26, 2017

@hudochenkov @jeddy3 maybe before v8 release we should remove unnecessary fixes don't related to standard syntax?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 27, 2017

@evilebottnawi there is no unnecessary fixes, that I'm aware of.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 27, 2017

@hudochenkov it is good 👍

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 27, 2017

"comment-empty-line-before": [ "always", {}, {fix: "path/fix.js"}]

@evilebottnawi: This sounds like a pain in the neck, and I doubt it would be used very often if you need different modules for each rule.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 27, 2017

@davidtheclark it is just idea 😄 , main thing here #2529 (comment)

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 27, 2017

I think if we can add fixing to a rule, we should fix as much as we can. Maybe we can't do fixing for some options of a rule. It's ok, we'll show linting error as usual. Or if we can't be sure about consistency of fixing for particular options or rule, we won't add fixing, because it could be auto-breaking instead of auto-fixing.

Let's return to this PR :)

What should I do with part which checks code using style-search? Shall I rewrite it without style-search?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 27, 2017

@hudochenkov Run away from it 🏃 😛 If seriously we should get rid of it as much as possible where possible.

@hudochenkov hudochenkov changed the title from [WIP] Indentation autofixing to Indentation autofixing May 7, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented May 7, 2017

I haven't found a way how to rewrite this rule away from style-search, because it's very complex. But I finally figured out how to add autofixing for the code that uses style-search!

Less is still broken, because of semicolon bug in postcss-less. That's why tests without fix: true for it.

@stylelint/core PR is ready for a review.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented May 7, 2017

@hudochenkov i think before we should fix semicolon bug in postcss-less 😄 btw seems goods

@jeddy3 jeddy3 referenced this pull request May 17, 2017

Closed

Prettier for CSS #2532

@hudochenkov hudochenkov referenced this pull request May 25, 2017

Closed

Release 7.11 #2573

5 of 5 tasks complete
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 1, 2017

Shall we merge this and #2577, and then get out experimental support for autofixing (for standard syntax)?

We're only a couple of issues away from getting 8.0.0 out too. Although adding autofixing still has some question marks around it, it feels like setting off down this path is a good idea as the end goal of allowing plugins to autofix (e.g. stylelint-scss and stylelint-order) seems solid.

@jeddy3

jeddy3 approved these changes Jun 2, 2017

LGTM

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 2, 2017

@evilebottnawi could you take a look at this PR in the next few day, please? :)

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 8, 2017

@hudochenkov Shall we merge this? Tests look good and I think it's better we get the ball rolling on this fix stuff than wait. We're also introducing it as experimental feature.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 8, 2017

@jeddy3 Yeah, let's do it.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jun 8, 2017

@jeddy3 @hudochenkov: Sorry for my absence. I'm afraid my time is being consumed these days and I don't have much left to dedicate to stylelint — so please don't wait on me to make any progress!

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 8, 2017

let's merge and release 7.11 🥇

@hudochenkov hudochenkov merged commit de74769 into master Jun 9, 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 decreased (-0.01%) to 95.461%
Details

@hudochenkov hudochenkov deleted the indentation-fixing branch Jun 9, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 9, 2017

Thank you for a review!

Updated changelog:

  • 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
    • indentation
    • rule-empty-line-before

For some reason Github named commit as “Create CHANGELOG.md”.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 9, 2017

I'm in a similar situation. When we started working on stylelint I was on a hiatus from agency work, but now my time is being consumed by non-open source work. I'm going to try to make some time to help get 8.0.0 out of the door, though. @hudochenkov Thanks for driving these PRs through!

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