idanarye / vim-dutyl

Coordinate D tools to work together for you
http://www.vim.org/scripts/script.php?script_id=5003
79 stars 13 forks source link

Syntastic integration #17

Open joakim-brannstrom opened 9 years ago

joakim-brannstrom commented 9 years ago

The Vim plugin syntastic have support for D by checking the compile errors from DMD.

A problem though for syntastic/D is that the import paths for DMD are often wrong. So the error messages are often times complaining about the first module import that isn't part of std.

A solution for this would be to reuse the information that Dutyl have gathered from D tools and supply that to syntastic/D.

A proof of concept that is working "for me".

    " Use information about import paths from Dutyl to correctly setup
    " syntastic
    let s:dmd_paths=''
    try
        let s:duty_importPaths=dutyl#core#requireFunctions('importPaths').importPaths()
        for s:path in s:duty_importPaths
            let s:dmd_paths=s:dmd_paths.' '.dutyl#core#shellescape(s:path)
        endfor
    catch "Ignore errors and simply don't use them
    endtry
    let g:syntastic_d_compiler_options = s:dmd_paths

What would you say about adding such an integration in Dutyl? It would of course required checking if the plugin syntastic is installed etc.

idanarye commented 9 years ago

The problem is when to run it. Unless we can run it right before syntastic invokes DMD, the user will have to restart Vim whenever they add a new dependency...

It would have been ideal if syntastic had hooks - e.g. it could have run

doautocmd User Synstatic_pre_DMD

or something, and Dutyl will have

aucmd User Synstatic_pre_DMD call dutyl#updateSyntasticDMDPaths()

to update the import paths.

Another advantage of this approach is that neither plugin need to explicitly check for the other's existence - nothing will happen if syntastic invokes Synstatic_pre_DMD without Dutyl, and without syntastic Dutyl will simply never act on that event.

joakim-brannstrom commented 9 years ago

Interesting solution. I didn't know how to solve the problem you are highlighting so I naively set the variable only once when opening the "first D file".

I'll have to try your solution. Looks neat. If it works out well then a pull request to Syntastic first :)

joakim-brannstrom commented 9 years ago

Yes, it works nicely :) https://github.com/joakim-brannstrom/syntastic/tree/event_path_update_pre_dmd https://github.com/joakim-brannstrom/vim-dutyl/tree/syntastic-int

idanarye commented 9 years ago

Looks nice. This kind of events, if invoked from all across syntastic, could probably benefit checkers for other languages - but that's for the syntastic maintainers to decide and worry about.

joakim-brannstrom commented 9 years ago

Your feedback on the above branches would be useful :) I've updated them with what I think is almost good enough. I'lll try them out for a couple of days to hopefully ensure most bugs are caught.

Syntastic/D behavior syntastic/Dlang have a built-in discovery of probable compiler flags. I chose to keep it for two reasons. It allows a graceful degradation from vim-dutyl to syntastic/D for compiler flags when opening a d-file not part of a dub project. Current users of syntastic/D will not be affected by my meddling (the most important factor imho).

Compiler flags When the flag g:syntastic_d_dmd_user_configuration is set syntastic/D allows more detailed control of compiler flags. I have namely migrated to using syntastic default way of handling globals, for example g:syntastic_d_dmd_pre_args/args/post_args etc. It should hopefully allow vim-dutyl to supply even better flags in the future, like -dip25 etc :)

idanarye commented 9 years ago

OK, I took a deeper look:

  1. Please don't use dutyl#core#requireFunctions to invoke updateCache. This mechanism is for the external API that each Dutyl module provides to the main Dutyl functions. The point of this complex mechanism is that if, for example, none of the dub files is present dutyl#core#requireFunctions('importPaths') will return an object with importPaths from dutyl#configFile. You don't want this kind of behavior here - you don't want some other module to be able to declare updateCache with better priority that dutyle#dub's importPaths will use instead of it's own...
  2. Are fasterImportPaths and fasterStringImportPaths really needed? I understand that you it's faster this way because it saves a call to updateCache, but does it really justify complicating the API? In practice the speed improvement shouldn't even be noticeable because all it saves is checking the modification times of the defining files, which shouldn't change between the calls.
  3. The event hook shouldn't be registered in dutyl#syntastic#new. This function is only created on the first usage of Dutyl - if the user call syntastic before it uses anything else from Dutyl it won't catch the event and won't be able to fill the paths.
idanarye commented 9 years ago

If calling updateCache twice really does causes a noticeable penalty to performance, you can utilize the fact that dutyl#core#requireFunctions creates a new object every time it's called, and set a flag there to indicate you have already updated the DUB-related cache.

BTW - don't you also need to pass the versions field from DUB? Since they can affect conditional compilation, it may cause trouble to syntastic if -versions are not passed properly.

joakim-brannstrom commented 9 years ago

Thank you for the feedback. Exactly what was needed to improve it :)

  1. Done. I was uncertain of what the right way according to the design in vim-dutyl to "call functions". With your comment here I now understand how it works. This information wouldn't hurt to save for future contributors.
  2. Done. You are right, unnecessary. Premature optimizations. It is a difference of ~5 or ~10 I/O ops.
  3. Done. Your comment also clarified for me the purpose of ...#new :)

I'm trying to first concentrate on the simple case but ensure that the changes to syntastic allows maximal configurability. The syntastic changes allow the following control (prefix g:syntastic_ddmd): <exe> <post_exe> <args> <fname> <post_args> <tail>

post_exe is "-c -of/dev/null" post_args is filled by vim-dutyl.

This allowed me to personally "hardcode" -dip25 by setting g:syntastic_d_dmd_args. So this first step I think it is "good enough" to leave it up to the user to set "args" to the compiler flags he/she wants. But in the future mm, possibilities :) Maybe new configuration options in .dutyl.configFile? I dunno, but we can do it without affecting syntastic more :)

idanarye commented 9 years ago

I was uncertain of what the right way according to the design in vim-dutyl to "call functions". With your comment here I now understand how it works. This information wouldn't hurt to save for future contributors.

Yea, I really need to add this to the docs. Dutyl should support plugins - all it's tools are written as plugins - so I really should document how to structure these plugins...

Your comment also clarified for me the purpose of ...#new :)

Should have said it before, but the autocmd should be moved to plugin/dutyl.vim. Vim won't load autoload/dutyl/syntastic.vim until it needs to run a function with the dutyl#syntastic# prefix.

joakim-brannstrom commented 9 years ago

Should have said it before, but the autocmd should be moved to plugin/dutyl.vim. Vim won't load autoload/dutyl/syntastic.vim until it needs to run a function with the dutyl#syntastic# prefix.

"The requirement specification left room for assumptions". Thanks for correcting me. Done :)

joakim-brannstrom commented 9 years ago

Update. I've rebased both branches to make them ready for pull requests. But I'm holding off until I've tested the integration some more :) (so far, so good)

idanarye commented 9 years ago

One more suggestion - if you remove the line

call dutyl#register#module('syntastic', 'dutyl#syntastic#new', 120)

from plugin/dutyl.vim, Vim will postpone the loading of autoload/dutyl/syntastic.vim until Syntastic_d_pre_DMD is invoked - which means it will never get loaded if syntastic is not installed. Also, it's meaningless to register a Dutyl module that doesn't provide any function...

Yes, this is also something I need to add to the requirement specification when I finally get to writing it...

joakim-brannstrom commented 9 years ago

Done. I thought it was strange to do a register on the module. Good you pointed it out.

idanarye commented 9 years ago

While I'm pointing out things to remove, dutyl#syntastic is not a registered Dutyl module, so dutyl#syntastic#new is unnecessary and so does it's s:functions.

joakim-brannstrom commented 9 years ago

Thank you :)

I've tried to summaries our conversation here in the following text.

Specification for Dutyl plugins

Dutyl is designed to contain sub-plugins which only accesses each other via a plugin agnostic interface.

How-to add a new plugin to Dutyl

When adding a new plugin to dutyl consider the following questions.

Does the new plugin provide a service that is used by other plugins in Dutyl?

If yes then you need the following: a) A ...#new function. b) Registering the service.

a) Look in for example autoload/dutyl/dub.vim for a good example of ..#new. In ../dub.vim you see the script variable s:functions. That is how the registering is performed.

function! s:functions.projectRoot() abort

b) Registering the service is done by adding the following line to plugin/dutyl.vim

call dutyl#register#module('dub','dutyl#dub#new',0)

Note the last parameter. It is the priority of the service. The design of dutyl allows a fallback mechanism. The priority determines the order of the fallback.

If no, then do NOT add any ...#new function, s:functions etc.

idanarye commented 9 years ago

Nice. I'll flesh it out some more when I have time.

I am actually pondering whether the plugin architecture documentation should be in markdown as part of CONTRIBUTING.md or in vimdoc as part of doc/dutyl.txt. Markdown is more friendly and can be displayed nicely both in Vim and on GitHub, but vimdoc integrates with the Vim help system. I also made a precedent in Vebugger, where I put the architecture documentation in the vimdoc...

I did a quick search and found a script for converting markdown to vimdoc, so it shouldn't be a problem to have both. I should convert Vebugger to do that too.

joakim-brannstrom commented 9 years ago

:)

In my ears CONTRIBUTING.md sounds right. It's not user documentation but rather developer. But I think either or works. A developer is happy when one can use the editor of choice to view the information.

Sweet, Vebugger looks nice. Another plugin to install!