Add rails_best_practices handler (resolves #655) (#751)

* Move FindRailsRoot() to more general location

* Add rails_best_practices handler (resolves #655)

* Update documentation for rails_best_practices

Also add brakeman to *ale* documentation.

* rails_best_practices: allow overriding the executable

* rails_best_practices: format help correctly

* rails_best_practices: capture tool output on Windows

9 files changed, 205 insertions, 23 deletions

diff --git a/README.md b/README.md
index 4f2bbf54..ccabc3a7 100644
--- a/README.md
+++ b/README.md
@@ -107,7 +107,7 @@ name. That seems to be the fairest way to arrange this table.
 | ReasonML | [merlin](https://github.com/the-lambda-church/merlin) see `:help ale-integration-reason-merlin` for configuration instructions
 | reStructuredText | [proselint](http://proselint.com/)|
 | RPM spec | [rpmlint](https://github.com/rpm-software-management/rpmlint) (disabled by default; see `:help ale-integration-spec`) |
-| Ruby | [brakeman](http://brakemanscanner.org/), [reek](https://github.com/troessner/reek), [rubocop](https://github.com/bbatsov/rubocop), [ruby](https://www.ruby-lang.org) |
+| Ruby | [brakeman](http://brakemanscanner.org/), [rails_best_practices](https://github.com/flyerhzm/rails_best_practices), [reek](https://github.com/troessner/reek), [rubocop](https://github.com/bbatsov/rubocop), [ruby](https://www.ruby-lang.org) |
 | Rust | [rustc](https://www.rust-lang.org/), cargo (see `:help ale-integration-rust` for configuration instructions) |
 | SASS | [sass-lint](https://www.npmjs.com/package/sass-lint), [stylelint](https://github.com/stylelint/stylelint) |
 | SCSS | [sass-lint](https://www.npmjs.com/package/sass-lint), [scss-lint](https://github.com/brigade/scss-lint), [stylelint](https://github.com/stylelint/stylelint) |
diff --git a/ale_linters/ruby/brakeman.vim b/ale_linters/ruby/brakeman.vim
index 5ea531f1..269c18f0 100644
--- a/ale_linters/ruby/brakeman.vim
+++ b/ale_linters/ruby/brakeman.vim
@@ -15,7 +15,7 @@ function! ale_linters#ruby#brakeman#Handle(buffer, lines) abort
 
     for l:warning in l:result.warnings
         " Brakeman always outputs paths relative to the Rails app root
-        let l:rails_root = s:FindRailsRoot(a:buffer)
+        let l:rails_root = ale#ruby#FindRailsRoot(a:buffer)
         let l:warning_file = l:rails_root . '/' . l:warning.file
 
         if !ale#path#IsBufferPath(a:buffer, l:warning_file)
@@ -36,7 +36,7 @@ function! ale_linters#ruby#brakeman#Handle(buffer, lines) abort
 endfunction
 
 function! ale_linters#ruby#brakeman#GetCommand(buffer) abort
-    let l:rails_root = s:FindRailsRoot(a:buffer)
+    let l:rails_root = ale#ruby#FindRailsRoot(a:buffer)
 
     if l:rails_root ==? ''
         return ''
@@ -47,26 +47,6 @@ function! ale_linters#ruby#brakeman#GetCommand(buffer) abort
     \    . ' -p ' . ale#Escape(l:rails_root)
 endfunction
 
-function! s:FindRailsRoot(buffer) abort
-    " Find the nearest dir contining "app", "db", and "config", and assume it is
-    " the root of a Rails app.
-    for l:name in ['app', 'config', 'db']
-        let l:dir = fnamemodify(
-        \   ale#path#FindNearestDirectory(a:buffer, l:name),
-        \   ':h:h'
-        \)
-
-        if l:dir !=# '.'
-        \&& isdirectory(l:dir . '/app')
-        \&& isdirectory(l:dir . '/config')
-        \&& isdirectory(l:dir . '/db')
-            return l:dir
-        endif
-    endfor
-
-    return ''
-endfunction
-
 call ale#linter#Define('ruby', {
 \    'name': 'brakeman',
 \    'executable': 'brakeman',
diff --git a/ale_linters/ruby/rails_best_practices.vim b/ale_linters/ruby/rails_best_practices.vim
new file mode 100644
index 00000000..a3d9f7a2
--- /dev/null
+++ b/ale_linters/ruby/rails_best_practices.vim
@@ -0,0 +1,59 @@
+" Author: Eddie Lebow https://github.com/elebow
+" Description: rails_best_practices, a code metric tool for rails projects
+
+let g:ale_ruby_rails_best_practices_options =
+\   get(g:, 'ale_ruby_rails_best_practices_options', '')
+
+function! ale_linters#ruby#rails_best_practices#Handle(buffer, lines) abort
+    if len(a:lines) == 0
+        return []
+    endif
+
+    let l:result = json_decode(join(a:lines, ''))
+
+    let l:output = []
+
+    for l:warning in l:result
+        if !ale#path#IsBufferPath(a:buffer, l:warning.filename)
+          continue
+        endif
+
+        call add(l:output, {
+        \    'lnum': l:warning.line_number + 0,
+        \    'type': 'W',
+        \    'text': l:warning.message,
+        \})
+    endfor
+
+    return l:output
+endfunction
+
+function! ale_linters#ruby#rails_best_practices#GetCommand(buffer) abort
+    let l:executable = ale#handlers#rails_best_practices#GetExecutable(a:buffer)
+    let l:exec_args = l:executable =~? 'bundle$'
+    \   ? ' exec rails_best_practices'
+    \   : ''
+
+    let l:rails_root = ale#ruby#FindRailsRoot(a:buffer)
+
+    if l:rails_root ==? ''
+        return ''
+    endif
+
+    let l:output_file = ale#Has('win32') ? '%t ' : '/dev/stdout '
+    let l:cat_file = ale#Has('win32') ? '; type %t' : ''
+
+    return ale#Escape(l:executable) . l:exec_args
+    \    . ' --silent -f json --output-file ' . l:output_file
+    \    . ale#Var(a:buffer, 'ruby_rails_best_practices_options')
+    \    . ale#Escape(l:rails_root)
+    \    . l:cat_file
+endfunction
+
+call ale#linter#Define('ruby', {
+\    'name': 'rails_best_practices',
+\    'executable_callback': 'ale#handlers#rails_best_practices#GetExecutable',
+\    'command_callback': 'ale_linters#ruby#rails_best_practices#GetCommand',
+\    'callback': 'ale_linters#ruby#rails_best_practices#Handle',
+\    'lint_file': 1,
+\})
diff --git a/autoload/ale/handlers/rails_best_practices.vim b/autoload/ale/handlers/rails_best_practices.vim
new file mode 100644
index 00000000..51bafbb0
--- /dev/null
+++ b/autoload/ale/handlers/rails_best_practices.vim
@@ -0,0 +1,6 @@
+call ale#Set('ruby_rails_best_practices_options', '')
+call ale#Set('ruby_rails_best_practices_executable', 'rails_best_practices')
+
+function! ale#handlers#rails_best_practices#GetExecutable(buffer) abort
+    return ale#Var(a:buffer, 'ruby_rails_best_practices_executable')
+endfunction
diff --git a/autoload/ale/ruby.vim b/autoload/ale/ruby.vim
new file mode 100644
index 00000000..503ff1db
--- /dev/null
+++ b/autoload/ale/ruby.vim
@@ -0,0 +1,22 @@
+" Author: Eddie Lebow https://github.com/elebow
+" Description: Functions for integrating with Ruby tools
+
+" Find the nearest dir contining "app", "db", and "config", and assume it is
+" the root of a Rails app.
+function! ale#ruby#FindRailsRoot(buffer) abort
+    for l:name in ['app', 'config', 'db']
+        let l:dir = fnamemodify(
+        \   ale#path#FindNearestDirectory(a:buffer, l:name),
+        \   ':h:h'
+        \)
+
+        if l:dir !=# '.'
+        \&& isdirectory(l:dir . '/app')
+        \&& isdirectory(l:dir . '/config')
+        \&& isdirectory(l:dir . '/db')
+            return l:dir
+        endif
+    endfor
+
+    return ''
+endfunction
diff --git a/doc/ale-ruby.txt b/doc/ale-ruby.txt
index 54ebbf5f..c710a26e 100644
--- a/doc/ale-ruby.txt
+++ b/doc/ale-ruby.txt
@@ -14,6 +14,27 @@ g:ale_ruby_brakeman_options                       *g:ale_ruby_brakeman_options*
 
 
 ===============================================================================
+rails_best_practices                            *ale-ruby-rails_best_practices*
+
+g:ale_ruby_rails_best_practices_executable
+                                   *g:ale_ruby_rails_best_practices_executable*
+                                   *b:ale_ruby_rails_best_practices_executable*
+  Type: String
+  Default: 'rails_best_practices'
+
+  Override the invoked rails_best_practices binary. Set this to `'bundle'` to
+  invoke `'bundle` `exec` `rails_best_practices'`.
+
+g:ale_ruby_rails_best_practices_options
+                                      *g:ale_ruby_rails_best_practices_options*
+                                      *b:ale_ruby_rails_best_practices_options*
+  Type: |String|
+  Default: `''`
+
+  The contents of this variable will be passed through to rails_best_practices.
+
+
+===============================================================================
 reek                                                            *ale-ruby-reek*
 
 g:ale_ruby_reek_show_context                     *g:ale_ruby_reek_show_context*
diff --git a/doc/ale.txt b/doc/ale.txt
index d8de056b..eafbbcc3 100644
--- a/doc/ale.txt
+++ b/doc/ale.txt
@@ -84,6 +84,8 @@ CONTENTS                                                         *ale-contents*
       pylint..............................|ale-python-pylint|
       yapf................................|ale-python-yapf|
     ruby..................................|ale-ruby-options|
+      brakeman............................|ale-ruby-brakeman|
+      rails_best_practices................|ale-ruby-rails_best_practices|
       reek................................|ale-ruby-reek|
       rubocop.............................|ale-ruby-rubocop|
     rust..................................|ale-rust-options|
diff --git a/test/command_callback/test_rails_best_practices_command_callback.vader b/test/command_callback/test_rails_best_practices_command_callback.vader
new file mode 100644
index 00000000..09436be0
--- /dev/null
+++ b/test/command_callback/test_rails_best_practices_command_callback.vader
@@ -0,0 +1,42 @@
+Before:
+  Save g:ale_ruby_rails_best_practices_executable
+
+  let g:ale_ruby_rails_best_practices_executable = 'rails_best_practices'
+
+  runtime ale_linters/ruby/rails_best_practices.vim
+  call ale#test#SetDirectory('/testplugin/test/command_callback')
+  call ale#test#SetFilename('../ruby_fixtures/valid_rails_app/db/test.rb')
+
+After:
+  Restore
+
+  call ale#test#RestoreDirectory()
+
+Execute(Executable should default to rails_best_practices):
+  AssertEqual
+  \ '''rails_best_practices'' --silent -f json --output-file /dev/stdout '
+  \   . ale#Escape(simplify(g:dir . '/../ruby_fixtures/valid_rails_app')),
+  \ ale_linters#ruby#rails_best_practices#GetCommand(bufnr(''))
+
+Execute(Should be able to set a custom executable):
+  let g:ale_ruby_rails_best_practices_executable = 'bin/rails_best_practices'
+
+  AssertEqual
+  \ '''bin/rails_best_practices'' --silent -f json --output-file /dev/stdout '
+  \   . ale#Escape(simplify(g:dir . '/../ruby_fixtures/valid_rails_app')),
+  \ ale_linters#ruby#rails_best_practices#GetCommand(bufnr(''))
+
+Execute(Setting bundle appends 'exec rails_best_practices'):
+  let g:ale_ruby_rails_best_practices_executable = 'path to/bundle'
+
+  AssertEqual
+  \ '''path to/bundle'' exec rails_best_practices --silent -f json --output-file /dev/stdout '
+  \   . ale#Escape(simplify(g:dir . '/../ruby_fixtures/valid_rails_app')),
+  \ ale_linters#ruby#rails_best_practices#GetCommand(bufnr(''))
+
+Execute(Command callback should be empty when not in a valid Rails app):
+  call ale#test#SetFilename('../ruby_fixtures/not_a_rails_app/test.rb')
+
+  AssertEqual
+  \   '',
+  \   ale_linters#ruby#rails_best_practices#GetCommand(bufnr(''))
diff --git a/test/handler/test_rails_best_practices_handler.vader b/test/handler/test_rails_best_practices_handler.vader
new file mode 100644
index 00000000..037d2527
--- /dev/null
+++ b/test/handler/test_rails_best_practices_handler.vader
@@ -0,0 +1,50 @@
+Before:
+    " Switch to the test rails directory.
+    let b:path = getcwd()
+    silent! cd /testplugin/test/handler
+    cd ../ruby_fixtures/valid_rails_app/app/models
+
+    runtime ale_linters/ruby/rails_best_practices.vim
+
+After:
+    " Switch back to whatever directory it was that we started on.
+    silent! 'cd ' . fnameescape(b:path)
+    unlet! b:path
+
+    call ale#linter#Reset()
+
+Execute(The rails_best_practices handler should parse JSON correctly):
+  silent file! thing.rb
+
+  AssertEqual
+  \  [
+  \      {
+  \          'lnum': 5,
+  \          'text': 'use local variable',
+  \          'type': 'W',
+  \      },
+  \      {
+  \          'lnum': 10,
+  \          'text': 'other advice',
+  \          'type': 'W',
+  \      }
+  \  ],
+  \  ale_linters#ruby#rails_best_practices#Handle(bufnr(''), [
+  \  '[',
+  \    '{',
+  \      '"message": "use local variable",',
+  \      '"line_number": "5",',
+  \      '"filename": "/testplugin/test/ruby_fixtures/valid_rails_app/app/models/thing.rb"',
+  \    '},{',
+  \      '"message": "other advice",',
+  \      '"line_number": "10",',
+  \      '"filename": "/testplugin/test/ruby_fixtures/valid_rails_app/app/models/thing.rb"',
+  \    '}',
+  \  ']'
+  \  ])
+
+Execute(The rails_best_practices handler should parse JSON correctly when there is no output from the tool):
+  AssertEqual
+  \ [],
+  \ ale_linters#ruby#rails_best_practices#Handle(347, [
+  \ ])