Max Offenses option

[mockdeep]

Is your feature request related to a problem? Please describe.

Our cops in .rubocop_todo.yml file have a tendency to just sit there forever. Even though we'd like to fix them, we never seem to find the time to devote to it. We could configure them in .rubocop.yml, but if there are too many offenses the rule will still end up disabled entirely when we auto-generate the configuration.

Describe the solution you'd like

It would be nice if --auto-gen-config could output with a MaxOffenses configuration on each rule, based on what is currently the "Offense count" comment. This would help ensure that people aren't introducing new violations on rules we'd like to enable, and over time as existing code is updated, the count of offenses will slowly decline. We regenerate our configuration pretty regularly, so the MaxOffenses configuration would, in theory, go down over time.

Describe alternatives you've considered

One option is to increase the --exclude-limit until we have only Exclude in .rubocop_todo.yml. This wouldn't prevent introducing violations in the excluded files, but it would prevent introducing them into files that aren't excluded. I'm also looking at specifying rules more precisely in .rubocop.yml to reduce the number of violations to the point where hopefully it ends up with Exclude instead of Enabled: false.

[jonas054]

Hi @mockdeep! This seems to be the same feature that you requested some six years ago in #1004. I'm curious to know; is it the same project where you're still struggling to get the offense count down?

Maybe I'm softer today than I was back then, so I'd like to give this request some consideration. Not sure if others agree. It mostly depends on how hard it is to implement.

[mockdeep]

@jonas054 Ha, I had totally forgotten about that. I am working on the same project, and it's an order of magnitude more code than it was back then. Though I'd also love to have this feature for other projects. It's nice to be able to set an intention even when refactoring the existing code isn't the top priority.

Reading over my other comments, I don't think hash rockets is the best example because it is auto-fixable. Those sorts of transitions are pretty easy to break up and get through in chunks. However, there are a number of rules that can't be auto-fixed, and those are much harder to enforce progressively. The existing violations need to be considered on a case-by-case basis to ensure that we don't break the code when refactoring it. But we still want to enforce it for new code even if the old stuff is invalid.

A lot of the Metrics rules have this problem. If there are lots of violations, we don't have a very clean way to limit CyclomaticComplexity for new code without having to refactor all of the existing code. For now we can increase the --exclude-limit and use --auto-gen-only-exclude option, but this can really bloat the .rubocop_todo.yml file. If we could plug the "Offense count" into a MaxOffenses setting inside .rubocop_todo.yml, I think that could actually replace the various other settings in the file. Max, Exclude, and Enabled: false could all become unnecessary.

[jonas054]

Please take a look at the branch I pushed above. I hope you can try it out for a bit and give me some feedback. Some things to note:

1. I prefer AllowedOffenses over MaxOffenses. It's more clear IMO.
2. Introducing a change like this is not easy. I chose the strategy of implementing it in the formatters, so that only the final stage of inspection is affected. I think it would be even more difficult trying to update code closer to the core.
3. The chosen strategy means that offenses are hidden from view rather than excluded from the outset. It has some undesirable consequences, namely that the exit code will be 1 even if RuboCop reports "no offenses detected", and rubocop --fail-fast will stop when the first file with any offenses, including allowed ones, is inspected.

[mockdeep]

@jonas054 wow, thanks for digging into this! I ran it on our codebase. It output a massive .rubocop-todo.yml. It seems to have missed some rules for some reason. When I ran rubocop again afterwards it had a lot of complaints still from rubocop-rspec. (Guessing probably just an artifact of your branch being a MVP spike.)

The new format of the .rubocop-todo.yml creates a lot of noise, though, and I think it's harder for it to be useful as a reference of rules we want to enable. I guess if someone wanted to improve things file-by-file it would be a good option, but in my case I generally think of things rule-by-rule. It's nice to be able to just delete a rule from the file and run rubocop to see what needs to be fixed.

I'm thinking about an alternative approach that might work out alright for us (and be no work for you). Basically, I think I might create a .rubocop_transitional.yml and run that against the most recent commit in CI. It means engineers will have to "prefactor" the files they touch to be inline with those styles. .rubocop.yml would be the absolute styles that every file needs to adhere to and .rubocop_transitional.yml is the aspirational rules. .rubocop_todo.yml would still serve its current purpose. We'll probably only add a couple of transitional rules at a time to avoid too much frustration.

It's not perfect, since it won't catch everything if there are multiple commits on a branch, but it seems like a good option to nudge things along in the right direction.

[mockdeep]

Something like:

git diff HEAD~1 --name-only --diff-filter=d | xargs rubocop -c .rubocop_transitional.yml

[jonas054]

Thanks for the feedback, @mockdeep! I'm glad you found an alternative solution that works for you, because it looks like it's going to be hard to come up with a solution in RuboCop that suits your needs without making unacceptable changes to how RuboCop operates.

The huge todo file with allowed offenses per file is necessary as far as I can see. Because if you set a global number of allowed offenses for a cop in a project, it only works when you're inspecting the entire code base. Checking a single file, for example, would probably not report any offenses even if new offenses had been added to it. So that's no good.

Be that as it may, I kind of like the new feature myself. Maybe I'll submit a PR. The problems with exit code and --fail-fast were not as hard to solve as I thought. We'll see.

[jayshepherd]

How about a command line flag to only exit with a code 1 if > n offenses are found. ex: --allowed-offenses 25

Use case:
We have a large codebase that is introducing Rubocop and has a high number of offenses. We'd like to not slow progress for now, but eventually reduce the number of allowed offenses over time. As a command line option, Jenkins could allow the build to continue based on a tolerated level.

[dvandersluis]

Why not just put all the existing offenses into the todo file then? New offenses could still be reported on and you could work through your todo file as time allows.

[mockdeep]

In some cases we have thousands of offenses, so the file would become huge. Adding exceptions to the todo file also means entire files will be excluded, which doesn't help when editing those files. What would be really handy is if there was an easy way to lint only the range of lines in a diff.

[bbatsov]

The problem with linting a range of lines is that those are unlikely to be complete expressions. You'd still be parsing the whole file and just reporting whatever is in the diff, which I find a bit weird. It can be done, but the API will be a bit weird as you will have to pass the diff ranges to RuboCop for each file.

[dvandersluis]

Yeah, I had an idea in the past about specifying line ranges in todo, but then as soon as the code changes in any ways the todo file will be broken.

[mockdeep]

Yeah, that sounds like a challenge. I think what would be handy is if there was a clean way to pipe the line numbers into RuboCop at run time that would be amazing. Though it's pretty hard to get the line numbers from git, and I can't imagine the API being very clean. It almost seems like RuboCop would need to have a direct integration with git and automatically handle that piece, too.

[dvandersluis]

Whether it's done internally to RuboCop or not it'd have the same challenges 😆 I think @bbatsov has mentioned before that we don't want to pick a VCS to support officially though.

Anyways like Bozhidar mentioned, even if we had a good way to do this, you'd have many situations like this:

 foo do
   something
+end
+
+bar do
+  something
   something_else
 end

and then the diff is not valid ruby syntax on its own and can't be processed by RuboCop.

test.rb:1:1: E: Lint/Syntax: unexpected token kEND
(Using Ruby 2.5 parser; configure using TargetRubyVersion parameter, under AllCops)
end
^^^

[mockdeep]

I guess what I was thinking is that RuboCop runs on the entire file, but then filters out any violations that are not part of the line numbers from the diff.

[dvandersluis]

Couldn't even easily do that to be honest. Cops look up and down in the node hierarchy all the time.

To give a contrived example, if there was a cop configured to require single statement blocks to be on a single line, the above diff would trigger that cop but it wouldn't be registered on any of the lines in the diff.

[mockdeep]

You know, to me, that seems like an acceptable tradeoff. When we're talking about transitional linters, it's less important to me that it be 100% perfect all of the time. I'm more concerned that on the whole, it's nudging us in the right direction. On the other hand, it might result in some confusion and people opening issues about it on your end.

Probably orders of magnitude more complex, but if rules had a range of lines they reference that might make it more robust.