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

Neovim integration for GoCoverage #686

Merged
merged 1 commit into from
Apr 2, 2016
Merged

Conversation

t-yuki
Copy link
Contributor

@t-yuki t-yuki commented Jan 16, 2016

Hi, I'm working on nvim integration of my coverage plugin: https://github.com/t-yuki/vim-go-coverlay
However, I've realized that go#cmd#GoCoverage is not integrated with nvim yet and several API changes are required for better integration.

Moreover, I guess that a recent change 579dd89 broke go#cmd#GoTestFunc because go#util#Shelllist also escapes "-run" flag.

So I want to include changes:

  • cmd: fix GoTestFunc is not working
  • cmd: show testing and test pass message in GoTest on neovim
  • jobcontrol: add AddHandler and RemoveHandler API to catch on_exit callback for callers of caller
  • cmd: add neovim integration for GoCoverage

However, I used ugly event handler method to track job status by go#cmd#GoTest and go#cmd#GoCoverage function. Another solution is welcome.

refs #607

@fatih
Copy link
Owner

fatih commented Jan 18, 2016

@t-yuki thank you a lot for your contribution! I'm going to look at it asap.

@fatih
Copy link
Owner

fatih commented Jan 19, 2016

@t-yuki the whole handler additions are, in my opinion, not needed. I think just the fix for :GoTestFunc would be OK. I've actually fixed it for plain vim, but neovim is still alpha and second citizen as support, I think that's the reason I didn't picked it up yet :) Is it possible for you to remove these two:

cmd: show testing and test pass message in GoTest on neovim
jobcontrol: add AddHandler and RemoveHandler API to catch on_exit callback for callers of caller

It's already complex right now and the message is OK in my opionin. Tough, I'm curious about your thoughts. Let me know what you think.

@t-yuki
Copy link
Contributor Author

t-yuki commented Jan 24, 2016

Sorry for late reply, ok, I'll resemble PR.
I agree with you that it is too complex.
I'll try to implement job control in my plugin.
If async job is supported on vim officially, let's reconsider it.

t-yuki added a commit to t-yuki/vim-go that referenced this pull request Jan 24, 2016
@t-yuki
Copy link
Contributor Author

t-yuki commented Jan 24, 2016

I wrote #695
However, It doesn't distinguish GoBuild job GoTest job because there is no context information in job struct.
To do this, adding completion event handler mechanism or attaching context information or callback handler information to jobcontrol#Span method as a argument will be required.
I think adding callback function pointer argument to jobcontrol#Span is more acceptable.

@fatih
Copy link
Owner

fatih commented Jan 27, 2016

Thanks @t-yuki. I'll look into this once we merge #695

@fatih
Copy link
Owner

fatih commented Mar 14, 2016

Hi @t-yuki do you still want to work on this? If yes could you please again rebase it? I'm more than happy to review it just let me know please! Thanks :)

…lback for callers of caller

cmd: add neovim integration for GoCoverage
@t-yuki
Copy link
Contributor Author

t-yuki commented Mar 19, 2016

Rebased, but not refactored as proposed in the above.
Also I removed showing test pass message because the master shows SUCCESS or FAIL in jobcontrol.
see: master...t-yuki:nvim-cov-wip#diff-a3998a862a20bc1665a15c84f401349fR185

Well, I'm looking for the way to work with latest vim's job_start function instead of nvim.
It's nice if jobcontrol can work with job_start, not only nvim.

@fatih
Copy link
Owner

fatih commented Mar 21, 2016

@t-yuki just tested it works well but if g:go_term_enabled=1 is set, it doesn't work. Did you test that case ?

@t-yuki
Copy link
Contributor Author

t-yuki commented Mar 23, 2016

Not yet.
I just tested and it works except it can't change current window using C+w so I can't return to original buffer, meaningless.
However, I guess it is reproducible in GoTest on master so it might not be originated in this change.

@svanharmelen
Copy link
Contributor

@fatih @t-yuki would be awesome if we could land this PR and the related vim-go-coverlay code 👍

@fatih fatih merged commit 19cfa6f into fatih:master Apr 2, 2016
@fatih
Copy link
Owner

fatih commented Apr 2, 2016

Thanks @t-yuki a lot for the PR. I really would like to merge vim-go-coverlay into this with some modifications. If you are OK I can create a PR with your changes (or you can do it), and we can go over it. Let me know what you think :) You did earlier that you would be OK merging it (#384 (comment)). I think we can go over the code and make it nicer and integrate with vim-go's codebase (which has many great utilities and base functions). I'm more than happy to work with you, just let me know.

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.

3 participants