Autocorrect?

[davidtheclark]

I thought I'd throw out this idea:

If a person wanted to build a library that looked for the same code style issues but instead of warning about them, corrected them automatically — that person would have to rewrite the same logic that we have to find the issues, then just take a different action once they're found. Right?

Because of that duplication, I'm wondering if we who have already written the initial logic might want to take the next step and provide an autocorrect option for each rule. Or it could even be another "severity" number: 0 = ignore, 1 = warn, 2 = err, 3 = autocorrect. We could integrate autocorrect features gradually, and only on rules where it could be done accurately.

I do think the reaction "That should be a separate tool" makes some sense; but because so much of the core logic & problem solving would be duplicated, I don't know if the pure module-separation strategy would be worthwhile in this case.

Thoughts?

[jeddy3]

that person would have to rewrite the same logic that we have to find the issues, then just take a different action once they're found. Right?

I think so.

Or it could even be another "severity" number: 0 = ignore, 1 = warn, 2 = err, 3 = autocorrect.

I like this idea.

We could integrate autocorrect features gradually

In color-hex-case and color-hex-length we're already calculating the correct value for the message. So, they seem like a good place to start. After that, it might be a good idea to try singular space rules like declaration-colon-space-after/before.

and only on rules where it could be done accurately.

Taking a quick look through the rules. I reckon the majority of the *-no-* and *-white/blacklist rules can't be autocorrected -- such as selector-no-id, rule-no-shorthand-property-overrides, rule-no-duplicate-properties and property-whitelist --, whereas the majority of the other rules can, including the whitespace ones.

[davidtheclark]

My reservation about the 0,1,2,3 idea is that then the numbers make less sense (when they probably don't make a lot of sense to some users to begin with). The "3" would mean they no longer correspond meaningfully to an exit code. If we wanted that flag to do all of this, I don't know, maybe we would be better off using keywords: "ignore", "warn", "autocorrect"?

Another thought I had: I imagine that for many people's builds autocorrection would be part of a new standalone process. Most of the time with PostCSS they are transforming code and outputting new files; but autocorrection would call for overwriting the original files. Also, most people probably include stylelint in their regular list of PostCSS plugins that ends up outputting a new file; so this would have to be a new PostCSS process with just stylelint autocorrecting. I guess that if stylelint has its own API/CLI that people use instead of adding it to an array of plugins, this will come naturally. Otherwise, I guess it's just a situation we'd have to document.

I'm curious what @MoOx has to say, and will try to solicit some input on the Twitter.

[jeddy3]

My reservation about the 0,1,2,3 idea is that then the numbers make less sense (when they probably don't make a lot of sense to some users to begin with).

Good point.

I don't know, maybe we would be better off using keywords: "ignore", "warn", "autocorrect"?

It certainly is clearer.

so this would have to be a new PostCSS process with just stylelint autocorrecting. I guess that if stylelint has its own API/CLI that people use instead of adding it to an array of plugins, this will come naturally.

Yes, I think so. It might be that autocorrecting is only enabled if stylelint is being used as a standalone tool, otherwise only linting is available.

I'm curious what @MoOx has to say, and will try to solicit some input on the Twitter.

Defo. It feels like it could be a contentious issue that would benefit from having a few more eyes cast over it :)

[MoOx]

I have to admit that autocorrect would be a nice feature. But making this "automatic" seems a bit crazy at first. Like you said, code will be weird (and string will be a pain for configuration imo).
Writing files automatically "all the time" seems a bit too much. New dev on a project might not understand why the file he is trying to save will change. Maybe I say that because this feature might changes habits.

JSCS have a similar feature and I think it's more reserved to stylistic rules (indent, whitespace etc) but maybe I am wrong. From my memory there is a jshint things to autocorrect some rules too.

I am not really for this feature, but at the same time, I am not against it. This will require some reflexions for a good UX in order to make this clear (bad ux might be "magic" & silent automatic changes without noticing the user). Maybe this would be a feature with a interactive mode? Or at least a feature with some (OS) notifications?

[davidtheclark]

I think I'm not understanding all your points, @MoOx ...

I think there's no suggestion to make it "automatic" or "all the time": you'd opt-in just like you would to any of the linting. One question is: What is the best way for users to opt-in via the config?

The user would definitely have to be aware of what's going on, or it wouldn't work. I don't think we would end up performing magic that would surprise them. They'd need a process set up to overwrite their existing files rather than run a PostCSS process that outputs new files.

Also, I agree that the changes shouldn't be silent: we could issue messages (not warnings) that say "Changing x to y".

I think this is the kind of thing that people would like to have in their text editor: when you hit save, it cleans up your little spacing mistakes and inconsistent hex capitalization, etc.

[MoOx]

What will happen if someone put a 3 in it's config that is "just" supposed to lint. Will it fix on runtime only? Will it overwrite the file?
I think an option should be separate (in the config) to say "hey, please fix warnings and errors and all you can fix so I can get my old crappy files a bit better".
This feature will probably be used not often when you get back on old code or when you setup the tool right?

[davidtheclark]

We could make the config able to lint and autocorrect at the same time. Currently, nothing will get overwritten, though, unless you set up a PostCSS process that does the overwriting. I guess once we have our standalone tool we should make it overwrite the file if you have any autocorrects on.

So you're suggesting a single option that turns on all the autocorrect possibilities at once? We could do that as well as having the ability to turn on each individually.

Yes, that would be useful to clean up old files; but I think people would also use it for stuff they are working on, so that instead of getting a warning when they miss a space it is just taken care of for them.

[MoOx]

I am just thinking about UX. Why someone would want to get something fixed during a build?

I see two use cases for a linter:

• just lint (warn) about potential issues (non breaking issues because those don't need a linter ^^) - real linting (daily usage via ci or in your editor - hello linter-stylelint!)
• taking some old code to adjust it according to a new standard: linting on steroid (casual usage)

Sure we can mix both, I just found this weird. The main purpose of a linter is to teach, not to fix, but I totally get the feature (and I would enjoy it for old crappy code I have to work on 2 years later!). I don't really think it's a good idea to allow something to fix mistakes automatically so easily, but maybe it's just my point of view. So if everyone think it's a good idea, do it :)

Btw, it will be easy with the upcoming cli tool to fix a folder directly.

Maybe we should take a look to jscs that have a similar issue (but this one is mainly a style checker so it just fix code style) http://jscs.info/overview.html#-fix-x. Doc is not really clear on the way this is behaving on files...

[davidtheclark]

I also would like to have my mistake pointed out and fix it myself. But I have a lot of teammates who would prefer the autocorrect, and I get that, too.

My reservation about the JSCS way is that users can't then pick and choose which rules get autocorrected, which don't. They just autocorrect everything. I guess maybe no users will want to pick and choose? If that's the case, we should just use a single universal setting like you suggested (which also could be set via CLI flag).

[MoOx]

The point of a linter is to ensure code quality. Since most of the time code is versionned, I don't see why keeping a non valid code (per linter configuration) might make sense. For example if I decide that a codebase won't have any vendor-prefix, I don't want a team mate to have them "fixed" (removed, then probably added back by autoprefixer, maybe with different values...). This seems weird to allow such things. But maybe I am missing something here.

JSCS by have no default rules on, so you need to define your code style. I think it make sense to use --fix to fix all the possible things in the scenario where you want to change code style.

[davidtheclark]

@jeddy3: Any thoughts on whether we should just have a blanket "fix" flag, or allow users to choose precisely which rules get autocorrected?

[jeddy3]

Any thoughts on whether we should just have a blanket "fix" flag, or allow users to choose precisely which rules get autocorrected?

I'd first like to revisit the rationale behind this feature as, after re-reading the discussion above, I'm now unsure whether we should implement it.

On a couple of occasions we've discussed the scope of this project and we've tended to make decisions that keep the project as narrowly-focused as possible. I think we've been right to be cautious of feature-creep as it's helped keep the project maintainable, manageable and approachable to new contributors.

We've also aimed for consistency at every opportunity, including the names, options, messages, and behaviour of each rule. I think this has helped create a good user experience.

We've added a few features recently, such as plugins and options validation. I think these features feel easily justifiable in their support of our goal to create a JavaScript-based pluggable linter for future and current CSS . Plugins were the key to allowing a user to extend the tool to support their processor plugin choices and any idiosyncratic aspects of their own coding styles. While the options validator instilled confidence in the user that the linter was configured correctly.

By contrast, I think this feature moves us away from being a linter and towards being a linter and a fixer. A fixer that will have inconsistent behaviour i.e. not all rules can be autofixed. I can envisage a user wanting to autofix everything, but being disappointed that it only works for some of the rules.

I think I'd rather see stylelint as a consistent, predictable and reliant linter, than a consistent linter with an inconsistent fixer built into it.

I think if the addition was an easy one to implement it might be easier to justify, but as this discussion has already shown and along with the discussions they seem to be having it at eslint it seems like adding autofixing is fraught with obstacles.

The points above apply to both use cases i.e. a user adjusting old code to a new standard and a user wanting to autofix code they're currently working on. I have a little more to say about the former use case...

When we started working on stylelint there weren't any JavaScript-based pluggable linters, with useful reporting, for future and current CSS out there. There are plenty of fixers though...

If you inherit an old code base and if you choose (via this comment) to clean up the existing styles, then there's plenty of tools out there to help you do this:

• https://github.com/morishitter/cssfmt
• https://github.com/csscomb/csscomb.js
• https://github.com/johnotander/postcss-remove-prefixes
• https://github.com/ben-eb/stylehacks

Not all these tools have the same depth of support for things like commas at the beginning of lines, but I think these are compromises you'd need to make if you choose to refactor an old code base.

Perhaps we should hold-off until we see how eslint clears some of the hurdles and how the other fixer tools evolve. I think then we'll be in a better position to decide if adding this feature to stylelint (along with the necessary support and inconsistency it will require) is worth it. What do you both think?

[davidtheclark]

Yeah, we can wait and see. I'm not attached to the autocorrect, idea, fine
with pushing it aside, but for the sake of discussion here I think are the
strongest arguments for doing it in stylelint:

• There are people who would prefer correction over linting warnings. Lots
  of people.
• I disagree that autocorrect behavior would introduce unusual or
  difficult-to-grasp inconsistency that would harm UX. I think it would be
  pretty straightforward: rules a-m can be autocorrected if you designate
  that you want that. rules n-z can only warn. If the user is disappointed
  because their vendor prefixes weren't autocorrected, that user is just
  going to have to read some documentation -- which they already had to do
  to create the config and setup the rule in the first place.
• stylelint is in a position to do a uniquely good job autocorrecting. Any
  module that wanted to autocorrect as many features as we could
  autocorrect would have to rewrite most of our code. (Unless we dramatically
  refactored, I don't think they could just reuse the parts of our code that
  would help them along.) Besides offering more formatting features and
  options than existing modules, stylelint could do it better than
  non-postcss solutions like csscomb (by maintaining all your existing code
  style except specifically the requested corrections).

All that said, it would be a lot of work. Feel free to close this issue if
both of you think autocorrecting is too much.

On Wed, Aug 5, 2015 at 9:39 AM, Richard Hallows notifications@github.com
wrote:

Any thoughts on whether we should just have a blanket "fix" flag, or allow
users to choose precisely which rules get autocorrected?

I'd first like to revisit the rationale behind this feature as, after
re-reading the discussion above, I'm now unsure whether we should implement
it.

On a couple of occasions we've discussed the scope of this project and
we've tended to make decisions that keep the project as narrowly-focused as
possible. I think we've been right to be cautious of feature-creep as it's
helped keep the project maintainable, manageable and approachable to new
contributors.

We've also aimed for consistency at every opportunity, including the
names, options, messages, and behaviour of each rule. I think this has
helped creat a good user experience.

We've added a few features recently, such as plugins and options
validation. I think these features feel easily justifiable in their support
of our goal to create a JavaScript-based pluggable linter for future and
current CSS . Plugins were the key to allowing a user to extend the tool to
support their processor plugin choices and any idiosyncratic aspects of
their own coding styles. While the options validator instilled confidence
in the user that the linter was configured correctly.

By contrast, I think this feature moves us away from being a linter and
towards being a linter and a fixer. A fixer that will have inconsistent
behaviour i.e. not all rules can be autofixed. I can envisage a user
wanting to autofix everything, but being disappointed that it only works
for some of the rules.

I think I'd rather see stylelint as a consistent, predictable and reliant
linter, than a consistent linter with an inconsistent fixer built into it.

I think if the addition was an easy one to implement it might be easier to
justify, but as this discussion has already shown and along with the
discussions they seem to be having it at eslint
eslint/eslint#3134 it seems like adding
autofixing is fraught with obstacles.

The points above apply to both use cases i.e. a user adjusting old code to
a new standard and a user wanting to autofix code they're currently working
on. I have a little more to say about the former use case...

When we started working on stylelint there weren't any JavaScript-based
pluggable linters, with useful reporting, for future and current CSS out
there. There are plenty of fixers though...

If you inherit an old code base and if you choose
http://www.linuxfoundation.org/content/41-pitfalls (via this comment
WordPress-Coding-Standards/stylelint-config-wordpress#1 (comment))
to clean up the existing styles, then there's plenty of tools out there to
help you do this:

• https://github.com/morishitter/cssfmt
• https://github.com/csscomb/csscomb.js
• https://github.com/johnotander/postcss-remove-prefixes
• https://github.com/ben-eb/stylehacks

Not all these tools have the same depth of support for things like commas
at the beginning of lines, but I think these are compromises you'd need to
make if you choose to refactor an old code base.

Perhaps we should hold-off until we see how eslint clears some of the
hurdles and how the other fixer tools evolve. I think then we'll be in a
better position to decide if adding this feature to stylelint (along with
the necessary support and inconsistency it will require) is worth it. What
do you both think?

—
Reply to this email directly or view it on GitHub
#330 (comment)
.