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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verilator current file search path #3500

Merged

Conversation

tarikgraba
Copy link
Contributor

@tarikgraba tarikgraba commented Dec 26, 2020

Adds the current edited file path to the linter search path.

As we are using a temporary file with verilator, we need to se the search path to the directory containing the original file.

@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch 3 times, most recently from ba9f6c4 to 48b57e6 Compare December 27, 2020 14:19
@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch 2 times, most recently from 74d74c7 to 7bfabff Compare January 6, 2021 10:42
@stale
Copy link

stale bot commented Feb 3, 2021

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 :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs a bot will close automatically label Feb 3, 2021
@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch from 7bfabff to 0a4f9f1 Compare February 5, 2021 10:33
@stale stale bot removed the stale PRs a bot will close automatically label Feb 5, 2021
@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch 2 times, most recently from 4f94da6 to 3b5021b Compare February 5, 2021 16:14
@tarikgraba
Copy link
Contributor Author

@w0rp Any hint about the error that I am getting here and which I can't reproduce locally?

(1/1) [EXECUTE] (X) Vim(let):E121: Undefined variable: g:call_count
      > function ale#Queue[33]..<SNR>27_Lint[20]..ale#engine#RunLinters[4]..<SNR>36_GetLintFileValues[27]..<lambda>803[1]..<SNR>36_RunLinters[12]..<SNR>36_RunLinter[2]..ale#lsp_linter#CheckWithLSP[1]..ale#lsp_linter#StartLSP[3]..ale#lsp_linter#FindProjectRoot[33]..ale#handlers#hdl_checker#GetProjectRoot[12]..ale#handlers#hdl_checker#IsDotGit, line 1
  Success/Total: 0/1

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.

I left some comments here. I have pushed a commit which should fix another test which wasn't cleaning up its state properly.

@@ -0,0 +1,27 @@
Before:
" We edit a dummyy file to have valid bufnr/filename
let g:fileDummy = tempname() . '.v'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let g:fileDummy = tempname() . '.v'
let g:file_dummy = tempname() . '.v'

Use snake_case for variable names.

return 'verilator --lint-only -Wall -Wno-DECLFILENAME '
\ . ' -I' . l:buffer_abs_path . ' '
Copy link
Member

Choose a reason for hiding this comment

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

Use %s:h in the command instead, which is replaced with the absolute path to the directory of the file by ALE, and which can be replaced with a mapped filename if you're using something like Docker with the g:ale_filename_mappings option. See :help ale-command-format-strings and :help g:ale_filename_mappings for more information.

@@ -0,0 +1,27 @@
Before:
Copy link
Member

Choose a reason for hiding this comment

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

See :help ale-dev for information on writing tests for linter definitions. Try to structure this more like other linter tests, such as test/command_callback/test_iverilog_command_callback.vader,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have read the doc earlier...

@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch from 3b5021b to e937041 Compare February 11, 2021 19:08
@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch from e937041 to 4e2f94d Compare February 11, 2021 19:10
@tarikgraba tarikgraba force-pushed the verilator-current-file-search-path branch from 4e2f94d to 6c0aa8c Compare February 11, 2021 19:23
@w0rp w0rp merged commit ea72d66 into dense-analysis:master Feb 11, 2021
@w0rp
Copy link
Member

w0rp commented Feb 11, 2021

Cheers! 🍻

@tarikgraba
Copy link
Contributor Author

👍

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