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 #2467

Merged
merged 7 commits into from Apr 13, 2017

Conversation

4 participants
@davidtheclark
Contributor

davidtheclark commented Apr 2, 2017

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.

@davidtheclark davidtheclark force-pushed the another-fixer-poc branch from 0e0ade4 to af4cc12 Apr 2, 2017

@davidtheclark davidtheclark force-pushed the another-fixer-poc branch from af4cc12 to 7c7ee09 Apr 3, 2017

@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 :)

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

This comment has been minimized.

@hudochenkov

hudochenkov Apr 3, 2017

Member

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

This comment has been minimized.

@davidtheclark

davidtheclark Apr 6, 2017

Contributor

👍

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")

This comment has been minimized.

@hudochenkov

hudochenkov Apr 3, 2017

Member

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.

This comment has been minimized.

@davidtheclark

davidtheclark Apr 6, 2017

Contributor

👍

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 3, 2017

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

This comment has been minimized.

Member

hudochenkov commented Apr 3, 2017

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

This comment has been minimized.

Member

jeddy3 commented Apr 3, 2017

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

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 6, 2017

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?

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

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 8, 2017

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

This comment has been minimized.

Member

jeddy3 commented Apr 8, 2017

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

This comment has been minimized.

Member

hudochenkov commented Apr 9, 2017

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

This comment has been minimized.

Contributor

davidtheclark commented Apr 9, 2017

@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 davidtheclark force-pushed the another-fixer-poc branch from 9784876 to 30ccf15 Apr 9, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 9, 2017

@hudochenkov: Addressed your comments.

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

This comment has been minimized.

@hudochenkov

hudochenkov Apr 9, 2017

Member

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

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 9, 2017

@davidtheclark nice! Thank you!

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 9, 2017

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:

  1. This option throws an error when code is piped to ESLint.
  2. This option has no effect on code that uses processors.

I think we should also disable autofixing in these situations.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 9, 2017

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

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 9, 2017

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

This comment has been minimized.

Member

hudochenkov commented Apr 10, 2017

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

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 10, 2017

@hudochenkov

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

That's it, I think.

@jeddy3

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

README.md Outdated
@@ -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.**

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

"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.
@@ -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.

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

Let's put the "Autofixable" in brackets.

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

@@ -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.

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

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

@@ -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.

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

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

@@ -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).

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

"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.

This comment has been minimized.

@hudochenkov

hudochenkov Apr 12, 2017

Member

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

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

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

- `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:

This comment has been minimized.

@jeddy3

jeddy3 Apr 12, 2017

Member

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

This comment has been minimized.

Member

hudochenkov commented Apr 12, 2017

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

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 12, 2017

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

- `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.

This comment has been minimized.

@jeddy3

jeddy3 Apr 13, 2017

Member

"the report()"

Soz, this mistake was my bad.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 13, 2017

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 hudochenkov force-pushed the another-fixer-poc branch from b271085 to 8fe9acc Apr 13, 2017

@hudochenkov hudochenkov merged commit a4b4f97 into master Apr 13, 2017

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
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.475%
Details

@hudochenkov hudochenkov deleted the another-fixer-poc branch Apr 13, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 13, 2017

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?

@evilebottnawi evilebottnawi referenced this pull request Apr 13, 2017

Closed

Happy news #291

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 13, 2017

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

@jitendravyas

This comment has been minimized.

Contributor

jitendravyas commented May 10, 2017

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

This comment has been minimized.

Member

hudochenkov commented May 10, 2017

@jitendravyas It's not released yet.

@jitendravyas

This comment has been minimized.

Contributor

jitendravyas commented Jun 9, 2017

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

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 9, 2017

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

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