igankevich / mesonic

Mesonic: A Vim plugin for Meson build system
46 stars 7 forks source link

Add syntastic integration #3

Closed Limeth closed 6 years ago

Limeth commented 6 years ago

This PR adds support for Syntastic linting for C via Meson/Ninja.

In my .vimrc, I additionally have the following, to enable this syntastic plugin only when there's a meson.build in the current directory.

autocmd FileType c call ConsiderMesonForLinting()
function ConsiderMesonForLinting()
    silent !test -f meson.build > /dev/null 2>&1
    if v:shell_error == 0
        let g:syntastic_c_checkers = ['meson']
    endif
endfunction
igankevich commented 6 years ago

Jakub, thank you for the PR!

Overall, it looks nice, but there are two things I'm not easy with.

  1. There are far too many non-portable shell invocations. Could you, please, use filereadable() and isdirectory() functions to check for file/directory availability? They are much faster and portable alternatives to shell functions.

  2. Please, replace hard-coded 'meson' and 'ninja' with g:meson_command and g:meson_ninja_command. These variables are already used by the plugin as mentioned in README. Mixing hard-coded commands and custom commands from variables may confuse users.

Limeth commented 6 years ago

I just tried using the g:meson_command and g:meson_ninja_command, but they seem to be undefined. As far as I can tell, these are the possible solutions:

Regarding the (test -d build > /dev/null 2>&1 || meson build) && ninja -C build command; I've done some research into Syntastic and I believe there isn't a way to register a function instead of a command to be called when highlighting is supposed to be updated. So I propose the following:

igankevich commented 6 years ago

OK. Let's change visibility of MesonCommand and NinjaCommand functions and call them from the syntax checker. It would be better to move both to plugin/meson.vim then.

As for the makeprgBuild, I would rather remove the whole exe_before part: Syntastic documentation says that these parts can be redefined by the user. Anyway, you provide error format for C errors only, and errors produced by running meson will not be detected. As far as I understand Syntastic, you would need another checker to catch and parse them.

Limeth commented 6 years ago

Anyway, you provide error format for C errors only, and errors produced by running meson will not be detected.

Yes, this script provides highlighting for C files. From my experience with Meson (I've only just begun using it), I haven't yet encountered any Meson warnings that should be displayed as syntax highlighting within C files. It may be possible to add highlighting for the meson.build file, but that would most likely fall under a different syntax checker plugin (possibly syntax_checkers/build/meson.vim).

igankevich commented 6 years ago

I haven't yet encountered any Meson warnings that should be displayed as syntax highlighting within C files.

There are no such warnings. Meson emit errors for meson.build files only. They look like this: https://github.com/igankevich/mesonic/blob/master/compiler/meson.vim#L62

It may be possible to add highlighting for the meson.build file, but that would most likely fall under a different syntax checker plugin (possibly syntax_checkers/build/meson.vim).

As far as I understand, this is the only clean way of checking meson.build errors with Syntastic. If you want, you may add this syntax checker plugin, and I will merge it in in one pull request.

The rest of the code is good enough to merge, I will do it as soon as you reply!

Limeth commented 6 years ago

I honestly don't know how I'd make a syntax checker for a file named meson.build instead of for all files with the .build extension. I think this functionality should fall under a separate PR anyway. I don't currently need it either, as my meson.build currently consists of 3 lines only.