Skip to content

Commit

Permalink
Fixed path matching with reek config
Browse files Browse the repository at this point in the history
Awhile back, I fixed reek to be able to know the filename being
examined, even if it wasn't passed a File object:
troessner/reek#1343

When the reek config file has different configuration for different
files/directories, this allows reek to match the filenames when not
using the reek CLI.

However, it wasn't working to match the full path as provided by
pronto-reek. For example, if your reek.yml specifies "lib/foo.rb",
pronto-reek was supplying files as
`Pathname("/home/me/code/project/lib/foo.rb")`, and it was failing to
match.

This change modifies the new_file_full_path from the patch to instead be
a relative path from the pwd, which is correctly matched by the reek
config.

Caveats:

 * Testing this might be pretty complicated, we'd need a `double` that
   knows a whole lot about Reek internals to generate a config.
 * Pronto::Git::Patch doesn't expose an easy way to get a relative
   path. I had to do the relative_path_from work twice in
   Pronto::Reek#run, this could be simpler if Pronto::Git::Patch exposed
   a `#new_file_relative_path` method.
 * I had originally wanted to do
   `patch.new_file_full_path.relative_path_from(Pathname.new(repo.path))`,
   but this did not work in the tests because the fixture git repo is in
   a subdir, so reek was unable to open "./hello.rb". While
   relative_path_from repo.path worked fine testing this against my own
   repository, I'm not familiar enough with how pronto treats submodules
   or nested git directories to know if that approach will work. I also
   don't know if `pwd` is the right approach either, depending on how
   reek/pronto is called. Probably a better thing to do is try and
   figure out the directory containing a reek config file, and do
   everything relative from there.
  • Loading branch information
paul committed Aug 28, 2019
1 parent 7d607ac commit 42f7cf9
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 5 deletions.
7 changes: 5 additions & 2 deletions lib/pronto/reek.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
module Pronto
class Reek < Runner
def run
files = ruby_patches.map(&:new_file_full_path)
files = ruby_patches.map do |patch|
patch.new_file_full_path.relative_path_from(Pathname.pwd)
end

configuration = ::Reek::Configuration::AppConfiguration.from_path(nil)

smells = files.flat_map do |file|
Expand Down Expand Up @@ -37,7 +40,7 @@ def new_message(line, error)

def patch_for_error(error)
ruby_patches.find do |patch|
patch.new_file_full_path.to_s == error.source
patch.new_file_full_path.relative_path_from(Pathname.pwd).to_s == error.source
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions spec/pronto/reek_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ module Pronto

context 'patches with additions to non-ruby files' do
let(:examiner) { double('examiner', smells: []) }
let(:ruby_file) { Pathname.new('ruby_code.rb') }
let(:other_file) { Pathname.new('other.stuff') }
let(:ruby_file) { Pathname.pwd.join('ruby_code.rb') }
let(:other_file) { Pathname.pwd.join('other.stuff') }
before { ::Reek::Examiner.stub(:new).and_return(examiner) }

let(:patches) do
Expand All @@ -51,7 +51,7 @@ module Pronto

it 'calls reek with only the ruby files' do
subject
::Reek::Examiner.should have_received(:new).with(ruby_file, hash_including(:configuration))
::Reek::Examiner.should have_received(:new).with(Pathname('ruby_code.rb'), hash_including(:configuration))
::Reek::Examiner.should_not have_received(:new).with other_file
end
end
Expand Down

0 comments on commit 42f7cf9

Please sign in to comment.