Add support for --stdin-filename #1343
Conversation
paul
force-pushed
the
feature/stdin-filename
branch
from
9ce6b6f to
2ca1882
Compare
|
This looks excellent to me. However travis is failing due to rubocop :) |
|
@troessner Yeah, I haven't had time to fix it. It's rubocop complaining about the complexity since I added the Probably the best thing to do is have sub-classes of The quick and dirty thing to make rubycop happy but the code probably worse would be to extract a couple helper methods, something like this: def self.from(source, filename: nil)
new(code: code_from_source(source), origin: origin_from_source(source, override: filename))
end...but, I personally think "private helper class methods" like |
|
@paul i agree - in this case you can add an exception to our Rubocop configuration |
| end | ||
| """ | ||
| Then it succeeds | ||
|
|
There was a problem hiding this comment.
For completeness, you should probably add 'And it reports nothing' as well.
| @@ -6,6 +6,10 @@ | |||
| reek_with_pipe(stdin, args) | |||
| end | |||
|
|
|||
| When /^I pass a stdin to reek *(.*) with:$/ do |args, stdin| | |||
lib/reek/examiner.rb
Outdated
| def initialize(source, | ||
| stdin_filename: nil, |
There was a problem hiding this comment.
Since we're in the process of releasing Reek 5, we could also require source to be a Source::SourceCode and avoid passing in this extra parameter.
Notice that stdin_filename is passed to Source::SourceCode.from and then read back through source.origin, so an alternative would be to not pass it to Source::SourceCode but store it and use it in Examiner#description.
| context 'when a stdin filename is provided' do | ||
| let(:app) { described_class.new ['--stdin-filename', 'foo.rb'] } | ||
|
|
||
| it 'assumes that filename' do |
There was a problem hiding this comment.
I don't see the specific filename being checked in the implementation of this spec?
There was a problem hiding this comment.
I should probably just remove this spec. I wanted somewhere in the regular specs (not just features) that would test the --stdin-filename option, and this seemed like the most logical place to do so. However, once I dug in, everything in here is so mocked out I wasn't sure how to actually test it, or even what I should be testing. I'm also surprised it passed, had it failed I probably would have just removed it.
lib/reek/source/source_code.rb
Outdated
| when Pathname then new(code: source.read, origin: source.to_s) | ||
| when String then new(code: source, origin: STRING_IDENTIFIER) | ||
| when String then new(code: source, origin: filename || STRING_IDENTIFIER) |
There was a problem hiding this comment.
Why is filename used with both IO and String? For STDIN use I think only IO would be needed, so if we're going to add this to others, we might as well add this to all of them.
However, see my comment on Examiner about not passing filename to SourceCode at all.
|
FWIW, I think 'private helper class methods' are perfectly fine. They're just hard to realize in Ruby. In this case, they could be pushed down into the instance and we could drop |
|
@paul will you be able to take a look at my comments soon? |
|
@mvz Yup, sorry, seems like my only free time these days is on weekends. I'll get to it today hopefully. |
paul
force-pushed
the
feature/stdin-filename
branch
from
47f566f to
7ab43bc
Compare
|
@mvz Changing However, making that change causes 500 specs to fail, since all the tests pass in things that are not SourceCode: But, I made it so that |
paul
force-pushed
the
feature/stdin-filename
branch
from
7ab43bc to
ac4da67
Compare
When other tools, like automatic linters within editors, run a sample through reek, they typically do so via stdin. However, if there are reek config exceptions to apply to that file, then reek has no way of knowing what the original filename was, triggering false positives. This adds an `--stdin-filename` CLI option that takes the original filename, and passes it through to the Examiner and SourceCode file to use as the `origin` attribute, allowing the configuration directives to apply.
Also allow `SourceCode.from` to noop if the provided source is already a `SourceCode`
paul
force-pushed
the
feature/stdin-filename
branch
from
ac4da67 to
aa2bea5
Compare
|
I had to rebase master to get CodeClimate to not error. Do you want me to also squash these into a single commit? I left it separate for now for easier review. |
There was a problem hiding this comment.
I think we should merge this and then see if any further refactoring is needed. @troessner wdyt?
|
Looks good to me as well, merging. Thanks @paul ! |
|
I'm trying to get Reek working with Ale (https://github.com/w0rp/ale), but the config in ---
# Directories below will not be scanned at all
exclude_paths:
- db/migrate
- tmpAnd the command... $ cat db/migrate/20181015212457_create_foos.rb | reek --config .reek.yml --force-exclusion --stdin-filename 'db/migrate/20181015212457_create_foos.rb'
db/migrate/20181015212457_create_foos.rb -- 3 warnings:
[6, 7, 8, 9, 11]:FeatureEnvy: CreateFoos#change refers to 't' more than self (maybe move it to another class?) [https://github.com/troessner/reek/blob/v5.1.0/docs/Feature-Envy.md]
[4]:TooManyStatements: CreateFoos#change has approx 9 statements [https://github.com/troessner/reek/blob/v5.1.0/docs/Too-Many-Statements.md]
[5]:UncommunicativeVariableName: CreateFoos#change has the variable name 't' [https://github.com/troessner/reek/blob/v5.1.0/docs/Uncommunicative-Variable-Name.md]Thanks! |
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.
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.
When other tools, like automatic linters within editors, run a sample
through reek, they typically do so via stdin. However, if there are reek
config exceptions to apply to that file, then reek has no way of knowing
what the original filename was, triggering false positives.
This adds an
--stdin-filenameCLI option that takes theoriginal filename, and passes it through to the Examiner and SourceCode
file to use as the
originattribute, allowing the configurationdirectives to apply.
Fixes #1296
@troessner I'm probably missing a few additional places where this might need to get additional tests. I added a cuke feature to start with, and a spec in
application_spec.rbthat passed as soon as I added the option, but before I implemented the changes inReportCommandandExaminer. Do you have any pointers on where to look that you'd want specs added?