-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: run opacheck on entire dir for more context #4531
base: master
Are you sure you want to change the base?
Conversation
ale_linters/rego/opacheck.vim
Outdated
@@ -11,7 +11,7 @@ function! ale_linters#rego#opacheck#GetCommand(buffer) abort | |||
let l:options = ale#Var(a:buffer, 'rego_opacheck_options') | |||
|
|||
return ale#Escape(ale_linters#rego#opacheck#GetExecutable(a:buffer)) | |||
\ . ' check %s --format json ' | |||
\ . ' check . --format json ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the command can start asynchronously, this could fire from a different buffer from the buffer you started on. Luckily, I added a quick way to get the directory of a file with fnamemodify
-like syntax a while ago.
\ . ' check . --format json ' | |
\ . ' check %s:h --format json ' |
Try this instead, and let me know if it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted in the documentation for ale#linter#Define
that by default ALE doesn't set a working directory for commands as that can actually break a lot of things. Another thing you could try is adding 'cwd': '%s:h'
to the arguments below, and then .
here should work. If you grep the codebase, a lot of linters explicitly set the work directory to be the directory of the file. Setting cwd
instead may or may not work better here, it's better to just run the linter and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for re-opening this - I had forgotten about it. It's been a while since I've done anything with opa
or rego
but I just tried this out. For a certain file I'm testing with, %s
throws some errors due to missing context from the rest of the directory, while both .
and %s:h
seem to work fine for my setup.
I can see in :ALEInfo
that using %s:h
shows the full path to the directory and that seems simpler to me than setting the cwd first. I pulled down the latest changes and updated that param
This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See |
Running `opa check` on a single file can report errors that might not exist when considering other files in the same directory
Running
opa check
on a single file can report errors that might not exist when considering other files in the same directoryrefs: #2531