Rails Best Practices Add Link to Warning #117
Conversation
ghost
assigned 
| end | ||
|
|
||
| def command | ||
| %Q(mf-rails_best_practices --format yaml --output-file #{output_file} .) |
There was a problem hiding this comment.
Hmm, I wonder if there's a better way to do this.
There was a problem hiding this comment.
Do you think you could require the libraries directly?
path = '.'
options = {'format' => 'yaml', 'without-color' => true, 'silent' => true} #output-file, with-git, with-hg, debug
analyzer = RailsBestPractices::Analyzer.new(path, options)
analyzer.analyze
# analyzer.output #writes to output-file
@output = analyzer.errors # [ Array<RailsBestPractices::Core::Error> ]
# useful methods on each error: type, url, message, filename, short_filename, first_line_number, git_commit, git_username, hg_commit, hg_username
There was a problem hiding this comment.
Yeah, don't even think you'd need the output_file/options stuff...
require 'rails_best_practices'
path = '.'
analyzer = RailsBestPractices::Analyzer.new(path,{})
analyzer.analyze
# analyzer.output
@output = analyzer.errorsThere was a problem hiding this comment.
You'll still want options = {'silent' => true} to turn off the progress bar
|
Thanks for the comments. I will take a look at this over the weekend, same for #118 |
|
On the other hand, it would not be inconsistent with the current implementation to do exactly what's in this PR. I think we just, in general, want to move away from shelling out. |
This link provides a description and a solution to the problem.
We now have the URL, so we can provide a link to rails-best-pracitces.com By calling the lib directly we can avoid IO overhead, however we want to be sure that the RailsBestPractices maintains the interface we are expecting, since it's not a publicly advertised interface. I have added some specs that will error if the interface to RailsBestPractices changes. huh
|
I think moving away from shelling out is a good direction to go. Since RailsBestPractices doesn't have a public interface I added a few tests that will fail if they change the interface that is being used. Let me know if you have any more thoughts. |
| mf_debug "** #{command}" | ||
| @output = `#{command}` | ||
| mf_debug "** Rails Best Practices" | ||
| path = '.' |
There was a problem hiding this comment.
This should probably be a configured option in the init.rb, so that it can be called here as options.fetch(:path)
I wonder if its worth including the 'silent' option in the init.rb, as well.
In any case, we should definitely allow other run options to be passed in and merged into {'silent' => true} (ensuring the keys are strings?) so that someone could run with git or hg integration if desired.
|
Looks awesome! Please rebase off of master and force push. We have the tests passing now git checkout master
git fetch upstream
git reset --hard upstream/master # or whatever your remote branch to metrcfu/metric_fu is called
git checkout rails-best-link-to-warning
git rebase master # there shouldn't be many problems. If there are, then git merge master
rspec # they should all pass now
git push -f origin rails-best-link-to-warning |
|
It would be great if you also updated the readme and/or .metrics file with how to configure by reference to, (thinking out loud)
|
|
I'm getting in interesting error when running all the test after rebasing with master from upstream. I get the following exception when running You can see from the stack trace that the Sexp#line method is being called with the wrong number of arguments. From what I can gather RailsBestPractices is requiring the CodeAnalyzer gem, which overrides Sexp#line and does not expect any arguments to be provided. This test will pass when I do not Any thoughts? |
|
Is it just a test failure? |
|
Nope. It breaks when running |
|
Wow, that is really annoying. The reason why it's even possible is because code_analyzer uses ripper and sexp_processor. If it used ruby_parser and sexp_processor, this hack would have failed a while ago. I'm not really sure what to do, to keep this code working, besides, for the moment, make sure rbp runs last and is required when it runs. |
|
bundle console file_sexp = RubyParser.new.parse(File.read('./lib/metric_fu/data_structures/line_numbers.rb'))
file_sexp.line => 3
require 'code_analyzer'
file_sexp.line => :module # and can no longer use RubyParser |
|
Saweet! Got the fixes merged in to rbp and code_analzyer and Richard Huang has released new versions:
You should be able to update the rbp in the gemspec now and it'll just work. -> |
|
Rebase off master and force push. It should work now. I put the rbp dependency in master 593f7ee |
|
Nevermind, I'll just do it from the commandline |
|
Congrats! |
This is in reference to #87.
I changed the format of the output of Rails Best Practices to yaml, so we can parse the url. This url provides a link to rails-bestpractices.com page, which describes the problem and provides a solution.