Generate rubocop_todo.yml

[garettarrowood]

This PR adds a rubocop_todo.yml file to root. This file was generated using the --auto-gen-config flag. This file can be leveraged to understand areas for "improvement" in the application code, and provide an easier path towards resolving offenses.

No worries if this is not something the rspec-core team wants to include. I noticed you recently upgraded to the most recent version of RuboCop.

If this is accepted, there are a few steps I'd recommend come next.

1. Add the cop Departments into rubocop_rspec_base.yml file. So instead of just MethodLength, this would be Metrics/MethodLength.
2. Alphabetize both rubocop config files to make it more user friendly when looking up your linting conventions. This also makes it easier to cross reference.
3. Revisit the mass disabling of rules during upgrade. These decisions can definitely be put off. But removing the falses and then regenerating this todo file presents a clear view of the current state of the project (in the view of rubocop, of course). I've found it had to make decisions about these all at once because there are 300+ cops with granular configurations. Often there is a setting for a cop that is preferable to disabling it. Not always though :).
4. Start addressing desired refactorings/fixes one group of related rules at a time, utilizing --auto-correct when possible.

Happy to do any of these things in follow up PRs. Also happy if you decide this doesn't sound like a good direction for the repo and this PR is closed :) .

[garettarrowood]

CI failed on rubocop linting in the first commit for 2 reasons. The 1st is resolved by this change. The EnforcedStyleForEmptyBraces configuration was defaulting to no_space when this codebase uses space.

[garettarrowood]

The second was this. There is one other place in the codebase that uses 6 method parameters. Since the todo detected this and raised the overall Max, the inline whitelisting became unneeded. Regenerating the todo reveals that the deletion I made raised the Offense count to 2.

Unfortunately, Metrics cops do not list exclusions the way all the other cop types do. So this is undesirable. I'll see if I can find the other offense and inline disable it.

[xaviershay]

Thank you!

I was the one who upgraded us. I'm not quite sure I follow what's going on here - how I thought things were set up was that we have a list of global ignores in .rubocop_rspec_base.yml (which is synced from rspec-dev, and then project specific things in .rubocop.yml. When upgrading, I created a TODO file like you have done and then "promoted" everything from that file into either the global file or the project specific one, so that by the end the rubocop build was green and the todo file was empty.

Does that match with your conception of how things are put together?

[garettarrowood]

Thanks for the fast feedback! The rspec-core architecture makes a little more sense now, but this also raises some questions about the todo counts I generated. Forgive my confusion as I plod along here, this is just going to be a jumble of thoughts/questions. Maybe when I finish writing, it will make more sense. :)

---

I am confused why CI wasn't picking up offenses that were not specified in either of the two current rubocop files. For example, my todo file collected Metrics/ModuleLength offenses. The Max got auto set to 2200 because it appears spec/rspec/core/configuration_spec.rb opens up a Module for that length. -- That's just one example, there are many more. The only way I know for a cop to be ignored is listing all offending files as Excluded in AllCops or the specific cop, or setting that cop to Enabled: false. I don't see Metrics/ModuleLength anywhere though.

Hmm. One other example is that my todo picked up a single Metrics/MethodLength offense of a method that is 134 lines. It looks like this offense is in spec/rspec/core/filterable_item_repository_spec.rb . Is your CI running rubocop only against files in lib? That is the default behavior for yard (but not for rubocop).

UPDATE

The BUILD_DETAIL.md specifies this. You do only run rubocop against files in lib... I guess I don't understand why other Ruby code in an application should not also follow coding conventions? Different conventions for spec files versus source code does make sense. The project could add another rubocop.yml inside the spec folder and set different config for these files. The main benefit for that are Metrics cops specifically. Other "Departments" of cops that enforce universally agreed on practices would be fine to inherit from a root level .rubocop.yml.

---

If the .rubocop_rspec_base.yml is synced, what is the rationale behind the upgrade that diverged it from rspec-dev? Happy to update the .rubocop.yml in rspec-dev with full Cop names if that is desired.

---

A thought on all the Disable: falses. I certainly don't agree with a lot of the rubocop defaults, but many configurations have been/are getting added to accommodate most existing styles. For me, it's less about what a style actually is, and more about just unifying the code in one style. If a rule is set to false, then styles a, b, c, d, e, etc.. all work. If a config can be found to support what a codebase already mostly does, that seems preferable. todos make those more obvious to me. And put off making a decision about every single rule in one PR.

---

Based on the revelation that rubocop is only run against lib, this todo file should definitely not be merged.

Does that match with your conception of how things are put together?

It does now :).

[xaviershay]

The project could add another rubocop.yml inside the spec folder and set different config for these files.

I'm not really sure the history behind excluding spec. Likely we didn't want the burden of maintain a separate set of cops. @rspec/rspec anyone remember?

what is the rationale behind the upgrade that diverged it from rspec-dev?

tl;dr I haven't finished the upgrade :) It was a PITA getting the upgrade done against all repos, I tried to keep them in sync and then intended to do a "final" rspec-dev sync once they're all merged. I haven't done that yet though coz still don't have rspec-rails merged. (Aside: I'm also preparing an RFC to merge all repos into a single one, which would make this kind of thing much easier.)

If a config can be found to support what a codebase already mostly does, that seems preferable. todos make those more obvious to me.

In general we've preferred to have "minimal" cop support, so only enable ones we particularly cared about. This isn't a strongly held project preference though I don't think. The upgrade introduced a huge number of new cops, so for expediency I basically defaulted to disabling them (i.e. I preferenced "get upgrade done" over "incorporate new cops that might be useful". Motivation for the upgrade was the recent CVE.)

Based on the revelation that rubocop is only run against lib, this todo file should definitely not be merged.

Thank you for the clarification, sorry that it was confusing :(

[xaviershay]

You know what, having a spec-only config that just disables everything is probably better than explicitly running the build against lib only. It might be slightly slower, but would be less confusing perhaps?

[garettarrowood]

My suggestion about having a second .rubocop.yml that disables many cops in /spec could work. But in hindsight, an easier approach would just be to Exclude this directory in the .rubocop.yml the project has now. This could be done with AllCops config:

AllCops:
  Exclude:
    -  'spec/**/*'

Doing so would allow the linting to catch all the Ruby files in /benchmarks. But, IMO, it doesn't make sense to exclude /spec entirely. As development happens all over the codebase.

I'll throw up another commit up or two that excludes all the Metrics cops in /spec and lets CI run on all files. We can at least see what that looks like.

[garettarrowood]

Besides what appears to be a fluke, but consistent, bundler error for Ruby 1.9.3 testing, CI is now green.

[garettarrowood]

Let me know if you'd like me to reopen, or do any rubocop logistics once the repos have been combined :).