[WIP] [POC] Add linter to CLI

[NickLaMuro]

With the pronto effort sort of not being done... "proto"...

(...more like DOA, but ﾟ｡･* . ﾟ☆ puns ☆ﾟ.*･ ｡ﾟ)

I wanted something to take over for the miqbot CLI proposal that I had, and since this is included in every repo now, this seemed like a good fit.

A majority of the code is gutted from the bot, and just slightly tweeked and repurposed here. Additionally, I think we could easily be remove that code from the bot now, and have it call out to this library, but I think it might make more sense long term to make the linter code a lightweight gem (or possibly just the git_service portion as something like rugged-code_diff).

Regardless, I just wanted to provide this change as is for a first pass review

Example run

Note: This test was run prior to ManageIQ/manageiq#20997 being merged, but I there was no change on Gemfile line 7, so I think there is still some work to be done here.

$ bundle exec manageiq-style  --lint
Gemfile:7:1: convention: Style/RescueModifier: Avoid using `rescue` in its modifier form.
Gemfile:100:28: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:179:23: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:179:51: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:192:16: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:192:51: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:202:32: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:202:49: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:219:15: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:226:29: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:226:51: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:237:20: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:237:52: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:263:26: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:263:51: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:266:23: convention: Layout/ExtraSpacing: Unnecessary spacing detected.
Gemfile:287:23: convention: Layout/ExtraSpacing: Unnecessary spacing detected.

An alternative run using the rubocop CLI for contrast:

$ bundle exec rubocop --format emacs Gemfile
/path/to/manageiq/Gemfile:7:1: C: Style/RescueModifier: Avoid using `rescue` in its modifier form.
/path/to/manageiq/Gemfile:100:28: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:179:23: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:179:51: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:192:16: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:192:51: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:202:32: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:202:49: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:219:15: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:226:29: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:226:51: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:237:20: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:237:52: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:263:26: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:263:51: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:266:23: C: Layout/ExtraSpacing: Unnecessary spacing detected.
/path/to/manageiq/Gemfile:287:23: C: Layout/ExtraSpacing: Unnecessary spacing detected.

[Fryguy]

This is such a cool idea! Really looking forward to this.

One question with respect to the bot. Is the idea, essentially, that the bot would run manageiq-style lint in the repo? If so, is it then a requirement for manageiq-style to be included in any repo watched by the bot? It would be nice if we didn't require that. That is, if it could run manageiq-style with or without it being in the repo's Gemfile/gemspec, that would be cool.

[NickLaMuro]

Is the idea, essentially, that the bot would run manageiq-style lint in the repo?

No, that wasn't the original intent.

I was more thinking it would call out to ManageIQ::Style::Linter::Rubocop.new(branch).run, like it already does now:

https://github.com/ManageIQ/miq_bot/blob/bb7e7ac05e46e69483731801ff4fdac85b794044/app/workers/concerns/code_analysis_mixin.rb#L24-L26

The only change is the branch argument would now have to change to probably something like branch.linter_args, and we would have to implement that to work with the new interface, but that was the only change.

The main changes (which is hard to see since there is no diff), is that I decoupled the ActiveRecord pieces from the original code, so you could just pass them in as an options hash. The interface can be whatever we want (Hash, Struct, PORO, etc.), but the point was to make git_service/* not rely on the AR pieces.

---

That all said, it might not be a bad idea for the bot to be able to call out to $ manageiq-style lint, since it is something where we could easily transition away from the bot if we wanted, but again, that wasn't the original intent.

The main reason I shoved it in this repo (to start) is we already had an executable this is basically in all of the repos, and was available via a bundle exec ... to anyone who has it pulled down. I have no problem with it working standalone, and pretty sure nothing would need o change to do that, but I could be missing something.

[Fryguy]

Nice. Yeah putting it in this repo is fine, and then we can figure out if it ultimately lives here, or lives standalone.

The main changes (which is hard to see since there is no diff), is that I decoupled the ActiveRecord pieces from the original code, so you could just pass them in as an options hash. The interface can be whatever we want (Hash, Struct, PORO, etc.), but the point was to make git_service/* not rely on the AR pieces.

Perfect! Decoupling that is a great move.

[NickLaMuro]

Random Side Musing: So something I was digging into (because it was bothering me) was why that line 7 was still in my output.

Turns out, there is a part of the "Linter" that wasn't actually part of the classes I pulled over:

https://github.com/ManageIQ/miq_bot/blob/bb7e7ac05e46e69483731801ff4fdac85b794044/app/workers/commit_monitor_handlers/commit_range/rubocop_checker/rubocop_results_filter.rb

Specifically, the filter_on_diff:

https://github.com/ManageIQ/miq_bot/blob/bb7e7ac05e46e69483731801ff4fdac85b794044/app/workers/commit_monitor_handlers/commit_range/rubocop_checker/rubocop_results_filter.rb#L25-L32

Which is what removes the lines that are not part of the diff.

There are two things here that are think should be done:

1. Probably move this into the Linter, since it's role is specific to the linting.
2. Remove this section since it seems like this should just be a rubocop config concern.

Regarding number 2, I think that is a relic that existed prior to the Linter class being introduced, so I think it just never got fixed.

---

Regardless, moving this logic over should fix the issue I was seeing where the lines parsed were everything wrong with the file in the source branch (when compared to the merge target), and not just what was part of the diff.