Skip to content

Added support for linting of proto files#1098

Merged
w0rp merged 8 commits into
dense-analysis:masterfrom
jeffwillette:add-proto-linter
Nov 10, 2017
Merged

Added support for linting of proto files#1098
w0rp merged 8 commits into
dense-analysis:masterfrom
jeffwillette:add-proto-linter

Conversation

@jeffwillette

Copy link
Copy Markdown
Contributor
  • added docs

I can add tests if they are necessary. I am not too familiar with vimscript so if tests are necessary some advice would be useful. Thanks

Comment thread ale_linters/proto/protoc-gen-lint.vim Outdated
\ 'name': 'protoc-gen-lint',
\ 'output_stream': 'stderr',
\ 'executable': 'protoc',
\ 'command': 'protoc --lint_out=. *.proto',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like *.proto is for checking files in the current directory. That will be expanded in the shell. Does this program read the file from stdin at all?

@jeffwillette

Copy link
Copy Markdown
Contributor Author

hmm there seems to be no options for reading from stdin. Is there any bash hackery that can be done to pipe stdin to it? I played around with some things but I couldn't get it to work

@w0rp

w0rp commented Nov 9, 2017

Copy link
Copy Markdown
Member

If the linter needs to look at files on disk, use 'lint_file': 1, so it only checks files on disk. Instead of something like *.proto, give it %s, so it checks just the file you're working on.

@jeffwillette

jeffwillette commented Nov 9, 2017

Copy link
Copy Markdown
Contributor Author

ok, I am now getting an error because the protoc needs the parent directory of the .proto file. Is there a format directive to get that? I tried doing %s/../ but it didn't like that :(

EDIT: I managed to get it working with the %s directive call by embedding a dirname arg into the command

EDIT2: I see from reading other PR's that this needs to be usable without bash specific commands so I'll work on writing a command_callback

Comment thread ale_linters/proto/protoc-gen-lint.vim Outdated
\ 'output_stream': 'stderr',
\ 'lint_file': 1,
\ 'executable': 'protoc',
\ 'command': 'protoc -I $(dirname %s) --lint_out=. %s',

@w0rp w0rp Nov 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You've got the right idea about using 'command_callback' here. What you should do is write a callback with 'command_callback' and add ale#Escape(expand('#' . a:buffer . ':p:h')) into the command to add the escaped directory name. That will work everywhere. Then add a test for the command callback in the test/command_callback directory. There are a few similar tests.

@jeffwillette

jeffwillette commented Nov 9, 2017

Copy link
Copy Markdown
Contributor Author

I have what I think is a working function here (just from looking at others and copying) but I get an error when I open vim that says

Error detected while processing function ale#Queue[9]..ale#CallWithCooldown[9]..<SNR>17_ALEQueueImpl[41]..ale#Lint[12]..ale#CallWithCooldown[9]..<SNR>17
_ALELintImpl[16]..ale#engine#RunLinters[12]..<SNR>102_RunLinter[7]..<SNR>102_InvokeChain[1]..ale#engine#ProcessChain[52]..ale#linter#GetCommand:
line    1:
E117: Unknown function: ale_linters#proto#protocgenlint#GetCommand
E15: Invalid expression: has_key(a:linter, 'command_callback')   ? ale#util#GetFunction(a:linter.command_callback)(a:buffer)   : a:linter.command

this is the file as it is when it gives the error. I have tried numerous things and nothing has gotten it to work

function! ale_linters#proto#protocgenlint#GetCommand(buffer) abort
    let l:dirname = expand('#' . a:buffer . ':p:h')
    let l:filename = expand('#' . a:buffer)

    "\   'command': 'protoc -I $(dirname %s) --lint_out=. %s',

    return 'protoc'
    \   . '-I' . ale#Escape(l:dirname)
    \   . '--lint_out=.' . ale#Escape(l:filename)
endfunction

call ale#linter#Define('proto', {
\   'name': 'protoc-gen-lint',
\   'output_stream': 'stderr',
\   'lint_file': 1,
\   'executable': 'protoc',
\   'command_callback': 'ale_linters#proto#protocgenlint#GetCommand',
\   'callback': 'ale#handlers#unix#HandleAsError',
\})

EDIT:

It was a file naming issue. It appears the file name needed to match the function name. vim script is weird

- added test for command_callback
Comment thread ale_linters/proto/protoc_gen_lint.vim Outdated

return 'protoc'
\ . ' -I ' . ale#Escape(l:dirname)
\ . ' --lint_out=. ' . ale#Escape(l:filename)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use just %s on the end there. ALE will automatically replace that with the filename.

Comment thread ale_linters/proto/protoc_gen_lint.vim Outdated
let l:dirname = expand('#' . a:buffer . ':p:h')
let l:filename = expand('#' . a:buffer)

"\ 'command': 'protoc -I $(dirname %s) --lint_out=. %s',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can remove this comment now.

Comment thread doc/ale.txt Outdated
pod...................................|ale-pod-options|
write-good..........................|ale-pod-write-good|
proto.................................|ale-proto-options|
protoc..............................|ale-proto-protoc|

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this entry and the documentation entry. There's no such linter with the name protoc.

@jeffwillette

Copy link
Copy Markdown
Contributor Author

@w0rp after the test failed I tried every which way I could think of to add %s on the end and nothing worked

. ' --lint_out=. ' . '%s'
. ' --lint_out=. ' . expand('%s')
. ' --lint_out=. '%s'
. ' --lint_out=. ' ale#Escape('%s')
. ' --lint_out=. ' . ale#Escape(expand('#' . '%s')

all failed for me

@w0rp

w0rp commented Nov 9, 2017

Copy link
Copy Markdown
Member

Put %s in the test too. %s is replaced later on by the engine.

@w0rp w0rp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, looks good. I'll add it to the README and the list in doc/ale.txt.

@w0rp w0rp merged commit 27780cb into dense-analysis:master Nov 10, 2017
w0rp pushed a commit that referenced this pull request Nov 12, 2017
* Added support for linting of proto files
* Added function to get the proper protoc command
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.

2 participants