Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scalastyle linter #766

Merged
merged 12 commits into from
Jul 13, 2017
Merged

Add scalastyle linter #766

merged 12 commits into from
Jul 13, 2017

Conversation

okkays
Copy link
Contributor

@okkays okkays commented Jul 12, 2017

Hello!

I haven't touched vimscript or vader before, so sorry for any issues that come up from me re-purposing code without fully understanding it 馃槄

Adds support for the scalastyle linter (www.scalastyle.org).

Adds an option, g:ale_scala_scalastyle_options = '-c scalastyle-config.xml', which allows custom options to the executable and provides a default config file name. (scalastyle requires -c be specified)

Has docs, and vader tests for the Handler and Command functions. I think I covered everything in the contributing page of the wiki. At the very least, I tried.

Thanks for taking a look!

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty good. Have a look at my comments.

" Description: Support for the scalastyle checker.

let g:ale_scala_scalastyle_options =
\ get(g:, 'ale_scala_scalastyle_options', '-c scalastyle-config.xml')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a common configuration file name? You should try and search for it in parent directories, and make it automatic. Look at the JSHint linter or other linters for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, although looking into it, it seems scalastyle.org defaults to scalastyle_config.xml while something (possibly sbt) has caused other projects to use scalastyle-config.xml. I've tried to account for both.

let l:patterns = [
\ '^\(.\+\) .\+ message=\(.\+\) line=\(\d\+\)$',
\ '^\(.\+\) .\+ message=\(.\+\) line=\(\d\+\) column=\(\d\+\)$',
\]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the continued lines on the same level of indentation, but up-indent the lines inside of the List. Have a look through the codebase for examples.


let l:col = l:match[4] + 0
if l:col > 0
let l:args['col'] = l:col + 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If l:col is 1, then the value here will be 2. I think these are off by one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed and tested. Scalastyle outputs zero-based column numbers, so it's now checking for the presence of a column with empty and adding one to get to vim's one-based indices.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me. I'll merge this now.

@w0rp w0rp merged commit 4c50aec into dense-analysis:master Jul 13, 2017
@okkays
Copy link
Contributor Author

okkays commented Jul 13, 2017

Awesome, thanks!

@w0rp
Copy link
Member

w0rp commented Jul 13, 2017

Cheers! 馃嵒

rsrchboy added a commit to rsrchboy/ale that referenced this pull request Jul 15, 2017
* upstream/master: (23 commits)
  dense-analysis#764 - Update the documentation to mention how to echo messages with the ALELint autocmd
  Simplify the tests for the ALELint autocmd command
  Add scalastyle linter (dense-analysis#766)
  dense-analysis#697 - Remove highlights more thoroughly
  dense-analysis#769 Ignore stderr output and output without JSON we can read for rubocop
  Make tsserver completion more reliable, but not quite ready for documentation yet
  Rubocop: handle empty 'files' array in output
  Fix dense-analysis#760 - Report problems with configuration files for rubocop
  Add rails_best_practices handler (resolves dense-analysis#655) (dense-analysis#751)
  Fix docs for FindProjectRoot function
  Fixup dense-analysis#756
  dense-analysis#756 Escape the paths used for the --include parameter for gometalinter, which uses RE2
  Fix documentation typo
  Fix dense-analysis#747 - Lint and fix files after they have been been written to disk, not during writing them
  Look for ini file to spot python project root (dense-analysis#755)
  Make gometalinter work again
  Prefer --fast for stack-build (dense-analysis#754)
  Fix comment typo
  Brakeman: Remove unused cache var from tests
  Kotlin and general Gradle support. (dense-analysis#745)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants