Proposal: Build on RuboCop infrastructure

[mvz]

This is something I've been thinking about for some time.

There have been quite a few long-standing issues in the tracker that are not about Reek's core functionality. These have to do with configuration, the todo file, and the inability to make Reek process files that do not end in .rb.

• Configuration-related issues: directories in config relative to cwd, not to configuration file itself. #1486, Directory configurations changed incompatibly in v5.4.0 #1482, Reek config inheritance #1434, Inherit reek config from a gem #1430, Piping source ignores excluded_paths configuration #1429, Use XDG standard for finding config files? #1417, Reek does not handle relative paths from config file #1283, and Clean up config handling #667.
• Inspecting files with different names: Include .thor files #1485, Somehow let Reek examine ruby files that don't have the .rb extension #721.
• Creating to do items: A strategy for making our --todo feature more useful #1411.
• Other: Piping source ignores excluded_paths configuration #1429, Validate the data types given to our basic and custom configuration options via source code comment #1145, Add reporting of unneeded inline suppressions #961.

In addition, there are quite a few different strategies being used for finding smelly code and we do not have one preferred way.

The rubocop project has done all the work of creating a working config file system with all the features our users have been asking for, a great to do file generation system, and an unneeded suppression detector. In addition, it has a nice system for matching s-expressions with certain patterns, as I mentioned before.

I would like to propose re-using some or all of these features for Reek. This would involve something like:

• Depend on rubocop
• Re-use rubocop's configuration loading code to read .reek.yml. This would mean adopting rubocop's file format
• Eventually port our code smell detectors to use rubocop's node matchers
• Perhaps port our detectors to be rubocop cops, so they can hook into the unneeded suppression cop system

A more extreme option would be to transform reek into a rubocop plugin (i.e., rubocop-reek), and keep the reek executable as a thin layer over that, just to load the correct configuration.

[troessner]

I think this is a fantastic idea well worth pursuing.

[elia]

This sounds great and very efficient, but not quite around the corner… 😊

Would you accept a PR that will always check against expanded paths meanwhile?
That should solve a bunch of the reported issues.

Directories in the config file would be expanded from the config file dir, and any passed file from the current dir.

[mvz]

@elia yes, PRs that fix outstanding issues are always welcome!

This proposal will definitely take some time. So I'm guessing it will probably be done for Reek 7.

[Drenmi]

Just a heads up. I plan to extract the parts of RuboCop that are essentially tools built on top of Parser, such as the node patterns and node extensions, into its own gem. I can't say when this will happen, but hopefully that will be helpful to this project as well. 🙂

[bbatsov]

One more vote in favour of this proposal. I think that sharing the same foundation between multiple lint tools will be beneficial for everyone. I've been thinking for a while for a gem like the one mentioned by @Drenmi (think of it as rubocop-ast) and perhaps a rubocop-core, which is everything except the ast logic and the actual bundled checks, but unfortunately I've made 0 headway in this direction for quite a while now. :-) Not to mention that this would mean a lot more overhead in terms of maintenance for RuboCop itself, as changes would need to happen in several gems and when you're short on time you're not exactly thrilled to create more work for yourself. :-) Anyways, I definitely think that's the long-term direction, but it's probably going to take us a while to get there. In the mean time I think it's perfectly fine to just depend on RuboCop directly.

[jdickey]

Came here looking for guidance on a parser problem; count me as supporting this as well. RuboCop has been solid in my experience for many years, and their opening up their infrastructure for others to build on is an unalloyed Good Thing that would no doubt mitigate problems like the following:

$ bundle install
# ...
Bundler could not find compatible versions for gem "parser":
  In Gemfile:
    reek was resolved to 5.2.0, which depends on
      parser (< 2.6, >= 2.5.0.0, != 2.5.1.1)

    skunk was resolved to 0.4.2, which depends on
      rubycritic (~> 4.0) was resolved to 4.4.1, which depends on
        parser (>= 2.6.0)

As of Release 0.81.1 [current], RuboCop depends on parser >= 2.7.0.1, which Should Just Work with Reek's current dependency. If and when RuboCop spin their non-core "platform-level" stuff off as an independently-versioned and -maintained Gem, it would make all kinds of sense for other somewhat-related tools like Reek to use that rather than maintaining their own outside dependencies. Yes?

[mvz]

@jdickey it looks like you're using an older version of reek. The current version will allow use with parser 2.6 and 2.7:

$ gem depend reek
[...]

Gem reek-5.6.0
  codeclimate-engine-rb (~> 0.4.0)
  kwalify (~> 0.7.0)
  parser (< 2.8, >= 2.5.0.0, != 2.5.1.1)
  psych (~> 3.1.0)
  rainbow (>= 2.0, < 4.0)

[mvz]

If and when RuboCop spin their non-core "platform-level" stuff off as an independently-versioned and -maintained Gem, it would make all kinds of sense for other somewhat-related tools like Reek to use that rather than maintaining their own outside dependencies. Yes?

I don't think so, unless they can actually use that platform-level stuff. Using such a gem purely for the transient dependencies would be overkill. There are several easy options for gems to stay up-to-date with their dependencies. It makes more sense to use those.

[jdickey]

@mvz About your first response: That's what I get; I'm pretty sure I'm using 5.6.0, as it's also the version listed in my Gemfile.lock.

$ gem depend reek
Gem reek-5.6.0
  codeclimate-engine-rb (~> 0.4.0)
  kwalify (~> 0.7.0)
  parser (< 2.8, >= 2.5.0.0, != 2.5.1.1)
  psych (~> 3.1.0)
  rainbow (>= 2.0, < 4.0)

About your second response: Yeah, I think you're probably right. It's been my occasional experience, though, that relying on a trusted, well-maintained base platform that offers a superset of the dependencies and basic behaviours you're after, is often a wise investment. I'd particularly note the (current) release frequency of Reek vs RuboCop; they do a bang-up job of getting fast fixes/updates out to the users (which I'd expect to slow down just a touch once they ship 1.0.0.)

(I just started some maintenance-level changes to a project I hadn't touched in just over a year, and was startled to realise that it last used RuboCop 0.59.2 (2018/09/24), and the current is 0.80.1. Yes, cleanup changes shaved the yak for a couple of hours.)

[mvz]

I'm pretty sure I'm using 5.6.0

That's odd, since your bundler output says:

    reek was resolved to 5.2.0, which depends on

I wonder what forces reek down to 5.2.0. Neither skunk nor rubycritic seem to force that. This is mostly a Bundler issue (it's not doing the best job explaining itself in this case), but you could try setting gem "reek", "~> 5.6.0" and see what it says.

RuboCop's release frequency is indeed quit a lot higher than Reek's. I'd like to think that is mostly due to its pre-1.0 status.

[jdickey]

That is odd, since Gemfile.lock insists that

    reek (5.6.0)
      codeclimate-engine-rb (~> 0.4.0)
      kwalify (~> 0.7.0)
      parser (>= 2.5.0.0, < 2.8, != 2.5.1.1)
      psych (~> 3.1.0)
      rainbow (>= 2.0, < 4.0)

and looking in the installed-Gem repository, one finds

$ ls ./tmp/gemset/gems/reek-5.6.0
CHANGELOG.md    Dockerfile      License.txt     Rakefile        docs            features        logo            samples         tasks
CONTRIBUTING.md Gemfile         README.md       bin             engine.json     lib             reek.gemspec    spec

Interestingly,

$ pwd
/Users/jeffdickey/src/ruby/conversagence
$ ls -l bin/reek
.rwxr-xr-x 787 jeffdickey 13 Mar  3:13 bin/reek
$ bin/reek --version
/Users/jeffdickey/src/ruby/conversagence/tmp/gemset/gems/bundler-2.1.4/lib/bundler/rubygems_integration.rb:374:in `block in replace_bin_path': can't find executable reek for gem reek. reek is not currently included in the bundle, perhaps you meant to add it to your Gemfile? (Gem::Exception)
	from /Users/jeffdickey/src/ruby/conversagence/tmp/gemset/gems/bundler-2.1.4/lib/bundler/rubygems_integration.rb:402:in `block in replace_bin_path'
	from /Users/jeffdickey/src/ruby/conversagence/tmp/gemset/bin/reek:23:in `<main>'

So Gemfile.lock and the Gem repository insist that Reek 5.6.0 is bundled, and the binstub exists, but the binstub (whether invoked directly or by bundle exec) says otherwise?

furious head scratching

[mvz]

Hm. I think we're firmly in rvm+bundler territory now. Maybe you can ask on one of those projects' support forums?

[jdickey]

I think you're likely right. Blast.

Thanks anyway.