Add autofixing #2467

davidtheclark · 2017-04-02T23:10:17Z

This builds off of #2427, but I thought it would be cleaner and a little easier for me to create a new PR, modifying things as I copy over changes, instead of adding another commit over there. Much of these changes are lifted from that PR and slightly modified.

This illustrates a way I think we could go about implementing autofixing. Here are the key elements:

New fix option in Node API, --fix in CLI.
Third argument, context, passed to rule functions. This is an object so that we can continue adding things to it, as needed. Right now it has fix: boolean, indicating whether to fix or not, and newline: \n | \r\n, determined by the first newline found in the file, which we'll use to add new newlines when fixing.
Code in at-rule-name-case and comment-empty-line-before that implements fixes for those rules.
Code in standalone.js that overwrites the input file, when fixing, with fixed CSS.
Modifications to jest-setup.js to check fixes if a test schema has fix: true. For accept tests, we always check that the fixed CSS equals the original. Every reject test must have a fixed property with the fixed CSS, and we check against that. All we need to add to tests, then, when we add fixing to a rule, is fix: true to each schema and fixed: .. to each reject test. This is illustrated in the modified rules.
System test ensuring both that the PostCSS Result is fixed and that the original file is overwritten with fixed CSS.

I tried via the CLI in a little local real-life test case, too, and it seemed to work!

What do you think @stylelint/core? Does this seem like a promising start? Please try it locally and give it some serious scrutiny.

After we all agree on our methodology, we should also try it out in an editor plugin, probably Atom, to ensure that the way we're doing it will work in that context.

hudochenkov

I like how you've revamped my proposal. I like how writing fixed files works from CLI and API. Your approach to test rules is what I had in mind, but didn't do :) It's great that you've added more necessary system tests.

Looks too good to be true :)

hudochenkov · 2017-04-03T05:53:59Z

lib/rules/comment-empty-line-before/index.js

+      // Fix
+      if (context.fix) {
+        if (expectEmptyLineBefore) {
+          comment.raws.before = /\n/.test(comment.raws.before)


It's better to extract this in a separate module, because there are many *-empty-line-before rules.

hudochenkov · 2017-04-03T06:13:17Z

jest-setup.js

+  const result = output.results[0]._postcssResult
+  return result.root.toString(result.opts.syntax)
+    // Less needs us to manually strip whitespace at the end of single-line comments ¯\_(ツ)_/¯
+    .replace(/(\n?\s*\/\/.*?)[ \t]*(\r?\n)/g, "$1$2")


Let's narrow this function to Less syntax only. I. e. apply this transformation only if test case has syntax: "less". This function could potentially mess with our tests results and give us false information.

jeddy3 · 2017-04-03T10:06:37Z

Does this seem like a promising start?

Very much so!

I think the simplicity of the changes to the rule code and tests is a big win here. It lowers the barrier to contributing autofixing PRs.

Please try it locally

Locally it worked great.

After we all agree on our methodology,

This route is looking very promising. The only thing that springs to mind is have we considered how two rules changing the same piece of CSS will work i.e. the "waves" approach in the automutate PoC?

hudochenkov · 2017-04-03T10:15:22Z

This route is looking very promising. The only thing that springs to mind is have we considered how two rules changing the same piece of CSS will work i.e. the "waves" approach in the automutate PoC?

How rules are working currently? One by one or in parallel? If one by one, then there is no problem. If in parallel, then there could be some problems. I don't know if two PostCSS plugins could work simultaneously on one AST.

If rules works in parallel, then we could force them in sync mode if fix enabled, to be safe.

jeddy3 · 2017-04-03T11:00:20Z

OK, scratch that query. I was thinking about something else, but I don't think it's relevant here.

davidtheclark · 2017-04-06T14:49:30Z

Great suggestions @hudochenkov. I'll try to get those fixes in soon.

In order to get this ready for release, we'll need documentation. Does anybody feel like contributing that to speed things along?

hudochenkov · 2017-04-08T20:14:27Z

I'll try to write docs tomorrow (on Sunday).

BTW, are we going to introduce this feature in 8.0? Then this PR should be based on v8 branch. Which is out of date with master a little bit.

jeddy3 · 2017-04-08T22:11:18Z

BTW, are we going to introduce this feature in 8.0?

It's non-breaking and so I think we can introduce this feature in the next minor release.

Should we flag it as an experimental feature?

hudochenkov · 2017-04-09T06:40:24Z

It's not non-breaking change, but it's a huge deal! I think it deserves to be in major release. Also minor releases less noticeable by users then major releases.

Also, 8.0 is pretty close.

davidtheclark · 2017-04-09T14:26:56Z

@hudochenkov: If we don't get it done before v8, then it goes out in v8; but I don't think that round version numbers would be a good reason to delay the experiment.

davidtheclark · 2017-04-09T16:35:57Z

@hudochenkov: Addressed your comments.

hudochenkov · 2017-04-09T16:44:50Z

lib/rules/comment-empty-line-before/index.js

+        if (expectEmptyLineBefore) {
+          addEmptyLineBefore(comment, context.newline)
+        } else {
+          comment.raws.before = comment.raws.before.replace(/(\r?\n\s*\r?\n)+/g, context.newline)


Could you, please, create a new helper function removeEmptyLineBefore? I didn't think about it at first, sorry.

hudochenkov · 2017-04-09T16:45:23Z

@davidtheclark nice! Thank you!

hudochenkov · 2017-04-09T17:14:26Z

I was looking to ESLint docs and found interesting thing about --fix:

Not all problems are fixable using this option, and the option does not work in these situations:

This option throws an error when code is piped to ESLint.

This option has no effect on code that uses processors.

I think we should also disable autofixing in these situations.

hudochenkov · 2017-04-09T18:12:36Z

I've just added docs. What should I change? What I've missed?

davidtheclark · 2017-04-09T22:02:39Z

I may not have time to work on this over the next week. If anybody wants to carry it through to completion, feel free; otherwise I'll try to get back to it in a week or two.

hudochenkov · 2017-04-10T05:38:01Z

@davidtheclark I want to carry it through. What do you think should be done before merge?

davidtheclark · 2017-04-10T14:14:51Z

@hudochenkov

Finish documentation (which it sounds like you worked on)
Make any code changes that are necessary

That's it, I think.

hudochenkov · 2017-04-10T17:48:47Z

I've added new utility removeEmptyLinesBefore for removing all empty lines before. Will be useful for *-empty-line-before rules.

@jeddy3 you're great writer. Could you, please, look at docs and check if everything ok?

@stylelint/core It's a big feature. Could you, please, take a look at it? I think we are ready to merge as soon docs are good.

jeddy3

Looking good to me. I've made a few suggestions.

jeddy3 · 2017-04-11T18:13:52Z

README.md

@@ -15,7 +15,7 @@ A mighty, modern CSS linter that helps you enforce consistent conventions and av
 -   **Understands *CSS-like* syntaxes:** The linter is powered by [PostCSS](https://github.com/postcss/postcss), so it understands any syntax that PostCSS can parse, including SCSS, [SugarSS](https://github.com/postcss/sugarss), and *experimental support* for Less.
 -   **Completely unopinionated:** Only enable the rules you want, and configure them with options that tailor the linter to your needs.
 -   **Support for plugins:** It's easy to create your own rules and add them to the linter.
-   **Automatically fix some stylistic warnings:** By using [stylefmt](https://github.com/morishitter/stylefmt) which supports stylelint configuration files.
+-   **Automatically fix some stylistic warnings.**


"Automatically fixes..." - for consistency with "Understand_s_ CSS-like syntaxes".

**Automatically fixes some stylistic warnings:** Save time by having stylelint fix your code with this *experimental* feature.

jeddy3 · 2017-04-11T18:16:49Z

docs/user-guide/rules.md

@@ -232,7 +232,7 @@ Here are all the rules within stylelint, grouped by the [*thing*](http://apps.wo

 -   [`at-rule-blacklist`](../../lib/rules/at-rule-blacklist/README.md): Specify a blacklist of disallowed at-rules.
 -   [`at-rule-empty-line-before`](../../lib/rules/at-rule-empty-line-before/README.md): Require or disallow an empty line before at-rules.
-   [`at-rule-name-case`](../../lib/rules/at-rule-name-case/README.md): Specify lowercase or uppercase for at-rules names.
+-   [`at-rule-name-case`](../../lib/rules/at-rule-name-case/README.md): Specify lowercase or uppercase for at-rules names. Autofixable.


Let's put the "Autofixable" in brackets.

Specify lowercase or uppercase for at-rules names (Autofixable).

jeddy3 · 2017-04-11T18:19:56Z

docs/user-guide/node-api.md

@@ -121,6 +121,10 @@ An absolute path to a custom [PostCSS-compatible syntax](https://github.com/post

 Note, however, that stylelint can provide no guarantee that core rules will work with syntaxes other than the defaults listed for the `syntax` option above.

+### `fix`
+
+If `true`, stylelint will fix as many errors as possible. The fixes are made to the actual source files. All unfixed errors will be reported. Not all errors are fixable using this option.


We can drop " Not all errors are fixable using this option.". It repeats the "as many errors as possible" bit from the first sentence.

jeddy3 · 2017-04-11T18:24:33Z

docs/user-guide/cli.md

@@ -70,6 +70,16 @@ stylelint "foo/**/*.scss"

 The quotation marks around the glob are important because they will allow stylelint to interpret the glob, using node-glob, instead of your shell, which might not support all the same features.

+### Autofixing errors
+
+With `--fix` option stylelint will fix as many errors as possible. The fixes are made to the actual source files. All unfixed errors will be reported. Not all errors are fixable using this option.


Drop: "Not all errors are fixable using this option."

jeddy3 · 2017-04-12T16:15:55Z

docs/developer-guide/rules.md

@@ -102,6 +102,34 @@ stylelint has a number of [utility functions](https://github.com/stylelint/style

 In particular, you will definitely want to use `validateOptions()` so that users are warned about invalid options. (Looking at other rules for examples of options validation will help a lot.)

+### Adding autofixing
+
+If you're sure it's possible to automatically fix all or some errors rule reports, you can add fixing functionality using [PostCSS API](http://api.postcss.org/) to modify PostCSS AST (Abstract Syntax Tree).


"Depending on the rule, it might be possible to automatically fix the rule's warnings by mutating the PostCSS AST (Abstract Syntax Tree) using the PostCSS API.

Determining the feasibility of autofixing a rule's warning is the responsibility of us all during the issue stage. It's not the just the rule author's responsibility.

Second sentence it's a sentence for docs or just a comment?

A comment. Sorry, I accidently missed off the closing quotation mark at the end of the first sentence.

jeddy3 · 2017-04-12T16:21:40Z

docs/developer-guide/rules.md

+-   `fix`(boolean): If `true`, your rule can apply autofixes.
+-   `newline`(string): Line-ending used in current linted file.
+
+If you can write fixing funtionality, then change `root` using PostCSS API and don't report that error:


If context.fix is true, then change root using PostCSS API and return early before the report() is called.

"If you can write fixing funtionality" repeat of above.

hudochenkov · 2017-04-12T19:30:04Z

@jeddy3 thank you! I've made changes. But have one question: #2467 (comment).

hudochenkov · 2017-04-12T21:00:39Z

@stylelint/core looks like everything is ready for merge. Any objections?

jeddy3 · 2017-04-13T12:52:38Z

docs/developer-guide/rules.md

+-   `fix`(boolean): If `true`, your rule can apply autofixes.
+-   `newline`(string): Line-ending used in current linted file.
+
+If `context.fix` is `true`, then change `root` using PostCSS API and return early before the `report()` is called.


"~~the~~ report()"

Soz, this mistake was my bad.

jeddy3 · 2017-04-13T12:55:27Z

Any objections?

Nope. As this is marked as an experimental feature, I don't see any harm in getting in front of users ASAP. We can add something to the changelog about how this release lays the foundation and currently only two rules are autofixed.

hudochenkov · 2017-04-13T16:28:30Z

I've merged it. Thank you, everyone! This feature is awesome! Don't release 7.11.0 yet. Give me day or two, I want to add autofixing for all *-empty-line-before rules.

Added to changelog:

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

@jeddy3 could you, please, change changelog if necessary?

davidtheclark · 2017-04-13T16:32:21Z

Thanks for pushing this through @hudochenkov. I'm excited to see how it works with users.

jitendravyas · 2017-05-10T07:37:30Z

I use grunt. Where to add fix: true? in gruntfile or stylelintrc file?

gruntfile

		stylelint: {
			all: ['app/styles/**/*.scss']
		},

Do I need to add it here?

		stylelint: {
			all: ['app/styles/**/*.scss'],
                       fix: true
		},

hudochenkov · 2017-05-10T07:45:37Z

@jitendravyas It's not released yet.

jitendravyas · 2017-06-09T13:33:01Z

@hudochenkov It's released now :-) Can you help me with my previous question?

hudochenkov · 2017-06-09T13:34:50Z

@jitendravyas :) I believe you should write to grunt plugin author about this ¯\_(ツ)_/¯

davidtheclark added the PR: review needed label Apr 2, 2017

davidtheclark force-pushed the another-fixer-poc branch from 0e0ade4 to af4cc12 Compare April 2, 2017 23:19

Another --fix proof-of-concept

7c7ee09

davidtheclark force-pushed the another-fixer-poc branch from af4cc12 to 7c7ee09 Compare April 3, 2017 00:04

hudochenkov requested changes Apr 3, 2017

View reviewed changes

jeddy3 added PR: needs documentation and removed PR: review needed labels Apr 7, 2017

jeddy3 changed the title ~~Yet another --fix proof-of-concept~~ Add autofixing Apr 7, 2017

jeddy3 mentioned this pull request Apr 7, 2017

Proof of concept: Add autofixing #2427

Closed

hudochenkov added the PR: needs revision label Apr 8, 2017

David Clark added 2 commits April 9, 2017 09:31

addEmptyLineBefore util

06e1603

Qualify Less adjustment

30ccf15

davidtheclark force-pushed the another-fixer-poc branch from 9784876 to 30ccf15 Compare April 9, 2017 16:35

hudochenkov reviewed Apr 9, 2017

View reviewed changes

Add docs

da9083e

hudochenkov added PR: review needed and removed PR: needs documentation labels Apr 10, 2017

jeddy3 requested changes Apr 12, 2017

View reviewed changes

hudochenkov approved these changes Apr 12, 2017

View reviewed changes

jeddy3 requested changes Apr 13, 2017

View reviewed changes

Update docs

8fe9acc

hudochenkov force-pushed the another-fixer-poc branch from b271085 to 8fe9acc Compare April 13, 2017 15:29

hudochenkov merged commit a4b4f97 into master Apr 13, 2017

hudochenkov added PR: review accepted and removed PR: review needed labels Apr 13, 2017

hudochenkov deleted the another-fixer-poc branch April 13, 2017 16:20

alexander-akait mentioned this pull request Apr 13, 2017

Happy news masaakim/stylefmt#291

Closed

hudochenkov mentioned this pull request Apr 13, 2017

Add autofixing for *-empty-line-before rules #2500

Merged

4 tasks

davidtheclark mentioned this pull request Apr 22, 2017

Proposal for review: suggested fixes #2096

Closed

gucong3000 mentioned this pull request May 16, 2017

Add autofix to *-no-vendor-prefix #2565

Closed

This was referenced Jun 9, 2017

Indentation autofixing #2529

Merged

Add autofixing for color-hex-case #2577

Merged

renovate bot mentioned this pull request Jun 9, 2017

Update dependency stylelint to version 7.11.0 singapore/lint-condo#310

Merged

dependabot-preview bot mentioned this pull request Mar 14, 2018

Bump stylelint from 7.10.1 to 9.1.2 nathanmsmith/RadioxDB-flatpage#26

Closed

Add autofixing #2467

Add autofixing #2467

Conversation

davidtheclark commented Apr 2, 2017

hudochenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddy3 commented Apr 3, 2017

hudochenkov commented Apr 3, 2017

jeddy3 commented Apr 3, 2017 • edited

davidtheclark commented Apr 6, 2017

hudochenkov commented Apr 8, 2017

jeddy3 commented Apr 8, 2017 • edited

hudochenkov commented Apr 9, 2017 • edited

davidtheclark commented Apr 9, 2017

davidtheclark commented Apr 9, 2017

Choose a reason for hiding this comment

hudochenkov commented Apr 9, 2017

hudochenkov commented Apr 9, 2017

hudochenkov commented Apr 9, 2017

davidtheclark commented Apr 9, 2017

hudochenkov commented Apr 10, 2017

davidtheclark commented Apr 10, 2017

hudochenkov commented Apr 10, 2017

jeddy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudochenkov commented Apr 12, 2017

hudochenkov commented Apr 12, 2017

Choose a reason for hiding this comment

jeddy3 commented Apr 13, 2017

hudochenkov commented Apr 13, 2017

davidtheclark commented Apr 13, 2017

jitendravyas commented May 10, 2017 • edited

hudochenkov commented May 10, 2017

jitendravyas commented Jun 9, 2017

hudochenkov commented Jun 9, 2017

jeddy3 commented Apr 3, 2017 •

edited

jeddy3 commented Apr 8, 2017 •

edited

hudochenkov commented Apr 9, 2017 •

edited

jitendravyas commented May 10, 2017 •

edited