Enable RuboCop auto-correction

[MaxLap]

So... I spent way more time than I wanted to on this. Folks from https://github.com/sds/haml-lint/issues/217 may be interested.

This draft PR adds auto-correction via RuboCop to Haml. It's not ready to be merged, but it's usable.

• Feature wise, it's pretty much finished.
• Test wise, there isn't much to add that I can think of.
• Code wise, there are some improvements that could be made... There always is.
• Documentation/comment wise, that was not my focus, so most of it is missing.

The system doesn't make a new parser for Haml. Here is a basic overview:

• Uses Haml's parser, but works around quirks by using the raw lines of interest from the haml file.
• First generates an array of "chunks" of ruby code that represent the Haml file.
• Not every chunks can be autocorrected, some are just placeholder for things like a "plain" to be able to correct the indentation and to avoid RuboCop thinking that an if is empty.
• The chunks are all written to a single file. Chunks that can be auto-corrected are wrapped with markers unique to the line in the file (ex: haml_lint_marker_62)
• Rubocop is then run onto that generated file, with a system to get the correct config.
• We then iterate over the chunks, find the corresponding new code using the chunks, and apply corrections back in the haml.

Can be used with:

• -a for safe auto-correct (which must be supported by your rubocop)
• -A for full auto-correct
• --auto-correct-only to ignore linters which don't support auto-correct (right now, only RuboCop does), and to ignore lint messages which were not auto-corrected (other than errors).
• There may be annoyances related to the Include/Exclude of your .rubocop.yml file. The linter writes a Tempfile with the .rb extension appended to your filename, so something like ....new.html.haml.rb.

Note: Be careful just running this one your views and pushing this. You better test, because Rubocop has bugs, and I probably didn't catch every one in this feature.

I've tried to split some misc stuff into the early commits. And the last commit is all of the tests. (Something like 4k lines)

Context: I felt I was in a very good position to write this. I had access to a codebase with with 80000 lines of haml, written mostly by juniors with very little oversight.. it includes every code pattern you can think of. I could quickly hook the test system to store the generated html files. So I could compare the before/after to make sure the results were identical (except for the empty lines).

I've already spend more than 2 months working on this in my free time. Much more than I hoped. At this point, it works. I ran this on the whole codebase. A few files have patterns that are not supported (which are just skipped). This auto-corrects 25k lints (mostly about quotes and hash-rockets). The (incomplete but close to 1000) tests all generated the exact same html (except for blank lines, which is expected).

My questions are:

• Is this feature likely to be accepted after it's been cleaned up and comments added?
• Is anyone willing to give a hand with finishing this?

[MaxLap]

Great, thanks for the positive answer and the code reviews. I have no problem supporting those changes.

I kind of burned myself out spending months on this already, I'll work on it when the spark come back a little.

Upgrading RuboCop is nice and I'm interested, but that also means new offenses in existing code. The most painful part of Rubocop is dealing with the Metrics cops. AbcSize, MethodLength, CyclomaticComplexity, PerceivedComplexity, ClassLength, etc. all feel painful, and I'm always wondering if it will be ok if I disable it for method X, Y, Z. I don't feel those cops add much but development overhead. Would you be open to me disabling them all entirely?

One thing I forgot to mention in the initial description is that if a file has lots of auto-corrections, the lints end up being in kind of meaningless locations, because as the Ruby file gets changed, RuboCop reports line numbers according to "after" the previous changes. So the basic mapping we use kind of gets broken. I have no idea if there is a solution to that and, to me, improving that is very low priority.

[baelter]

Nice work! Happy to help adding autocorrect for more linters.

[arrowcircle]

Thanks for your work! Hope this significant thing will be merged soon

[MaxLap]

Geez, it's already been 4 months. I do want to get back to this, but work, vacation and other projects have been leaving me with little time. Just wanted to show a sign of life on my side :)

[bloycey]

Unfortunately I don't have the skills to personally contribute to this PR but just wanted to say thank you for all your work on this so far. Really hoping it gets merged as it would be a huge time saver!

[MaxLap]

So, finally gotten around to finishing this. Lots of cleanups and improvements to overall usability of the feature. At this point, I believe the feature is ready for a merge or a more thorough review.

Again, I split the commits so that they are self contained. Most of the first ones are just small improvements to Haml-Lint that felt needed or were found while developing the feature. The feature itself is in the 1 of the last 3 commits, and the tests are in another one of the last 3 commits.. Those are big.

Other than reviewing, here are some questions I had:

• Which appraisals should we use? Right now, I added one for Rubocop 1.0, 1.25 and 1.50. Those all test with the same HAML version. It can be changed pretty easily.
• RuboCop supports the terms auto-correct and autocorrect in its options, but encourages autocorrect. When I made the feature, I don't think that was the case. Either way, should we support both or only one? Thoughts? Right now, the PR supports auto-correct only because I wanted to wait for your input.

Finally, I did not resolve every Rubocop offenses because:

• Not sure which version you want (I recommend going for the latest)
• Some changes may be things you would prefer to disable
• There are lots of new cops to choose to enable/disable
• The changes affect basically every file in the project and this PR feels big enough
• I have a hard time caring about all the Metrics cops

I did upgrade RuboCop to 1.0 since that's the minimum, but only applied most fixes to the code I'm adding without editing the rest of the project.

[MaxLap]

Force pushed a fix for the Rubocop config_file option, which failed when given a relative path. (All tests used absolute path, ops!) Thanks to @thomasbrus for finding, investigating and reporting the bug.

If anyone want to try this feature and see how it goes, it would be very helpful to get some feedback on this.

To install this the gem with the feature, add this to your Gemfile: gem 'haml_lint', github: 'maxlap/haml-lint', branch: 'autocorrect_rubocopv2', require: false

Then, you can run with autocorrection using either haml-lint -a --auto-correct-only or haml-lint -A --auto-correct-only depending on your preference based on RuboCop's safe vs unsafe autocorrection.

The --auto-correct-only makes it faster for the first use by skipping linters which cannot do autocorrection.

Make sure to run your tests afterward! This can be a lot of changes depending on the code-base and your rubocop configuration.

Any feedback is welcome, either here or to the email in my profile.

[baelter]

Works like a charm!

You should make up your mind about quotes and autocorrect that

gem 'haml_lint', github: "maxlap/haml-lint", branch: 'autocorrect_rubocopv2', require: false

:smile:

[MaxLap]

Haha @baelter, good catch 😄 I fixed it.

And thanks for the feedback that you had no issues!

[sds]

Sorry for the delay in response, @MaxLap. Appreciate your effort on this. Responses inline.

• Which appraisals should we use? Right now, I added one for Rubocop 1.0, 1.25 and 1.50. Those all test with the same HAML version. It can be changed pretty easily.

If there is a significant difference in the AST exposed by RuboCop such that an appraisal version for it is necessary, we can add one for each substantial change. If not, I would suggest not adding appraisals for RuboCop, and simply require RuboCop 1.x as a minimum requirement.

• RuboCop supports the terms auto-correct and autocorrect in its options, but encourages autocorrect. When I made the feature, I don't think that was the case. Either way, should we support both or only one? Thoughts? Right now, the PR supports auto-correct only because I wanted to wait for your input.

Comfortable with auto-correct only for now.

Finally, I did not resolve every Rubocop offenses because:

• Not sure which version you want (I recommend going for the latest)

I'm fine with that as well.

• Some changes may be things you would prefer to disable
• There are lots of new cops to choose to enable/disable

I'm supportive of disabling by default or accepting whatever autocorrections RuboCop implements—whichever is easier.

• The changes affect basically every file in the project and this PR feels big enough
• I have a hard time caring about all the Metrics cops

Fine with disabling the Metrics-related cops for the files you've added or methods you've modified.

I did upgrade RuboCop to 1.0 since that's the minimum, but only applied most fixes to the code I'm adding without editing the rest of the project.

Sounds great, thank you.

---

Overall I'm supportive of this change, just need the build to be green. I know this has been sitting around for a while (apologies again) but happy to merge and cut a release once the build is green. Thank you!

[MaxLap]

@sds Alright, everything is green.

I switched to Rubocop 1.50.2 since that's the last one supporting Ruby 2.6.

I left a single Appraisal for Rubocop 1.0. as a just in case.

I applied all of the auto-correction of Rubocop, disable a few that I felt didn't add anything (ex: missing call to super). And added some rubocop:disable for metrics related stuff.

[sds]

Thanks!