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.

Sign up for GitHub

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

Jump to bottom

Coverage overlay in buffers #785

Closed
wants to merge 21 commits into from
Closed

Coverage overlay in buffers #785

wants to merge 21 commits into from

Conversation

t-yuki
Copy link
Contributor

@t-yuki t-yuki commented Apr 3, 2016

As suggested by #686 , I've merged vim-go-coverlay repository simply and modified several conflicts.

I'd ask these points to merge:

  • Should we merge command mappings?
  • Should we squash it?
  • It includes testsuite using VimFlavor.
    • Should we remove it or use another test tool?
  • It includes several ugly hacks to implement overlay.
    • Colorize lines using matchadd
  • Known Problem

t-yuki and others added 21 commits November 25, 2014 02:11
@t-yuki
initial commit
12c0ca5
@t-yuki
fix to match inclusive
5b990f0
@t-yuki
open quickfix only when errored
38bfa6b
@t-yuki
unlimit on readfile
768ca02
@t-yuki
refactor and add tests
32b7a06
@t-yuki
refactor and add tests. delete matches selectively on Clearlay
319539d
@t-yuki
set higher priority for covered line to beat the red
aabd713
@t-yuki
ignore close parentesis
1262cfa
@t-yuki
add a screen shot
dac0c65
Make it easier to run coverlay as a BufWritePost command.
f3cc980 
=> Clear coverlay before rerunning.
=> If tests pass, show coverlay immediately.
=> If tests fail, show window with list of failures,
   but don't switch to line showing failure. Useful for TDD.
Ignore coverage info if no tests exist.
7685a2b
@t-yuki
code cleanup
70c8cf8
@t-yuki
coverlay all buffers at once
4d9e43c
@t-yuki
update vim-flavor
fabeb9d
@t-yuki
add experimental support for neovim async test cover. catch up latest…
674268f 
… vim-go behaivior
@t-yuki
add tests
0ba4cf2
@t-yuki
fix command mappings. thanks @svanharmelen
bd79496
@t-yuki
Merge pull request fatih#3 from t-yuki/neovim
9cc31c8 
Neovim
@t-yuki
fix tests
5518f61
@t-yuki
Merge remote-tracking branch 'coverlay/master' into vim-go-coverlay
67cbd56
@t-yuki
rename t/test.vim to t/coverlay.vim for merge
2934945
@t-yuki t-yuki changed the title Vim go coverlay Coverage overlay in buffers Apr 3, 2016
fatih
fatih reviewed Apr 3, 2016
View reviewed changes
ftplugin/go/coverlay.vim
command! -nargs=* -bang GoCoverlay call go#coverlay#Coverlay(<bang>0, <f-args>)
command! -nargs=* GoClearlay call go#coverlay#Clearlay(<f-args>)

" vim:ts=4:sw=4:et
Copy link
Owner

Choose a reason for hiding this comment

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

Some notes:

  1. Mappings should be moved to mappings.vim
  2. Commands should be moved to commands.vim
  3. Commands should be renamed to:
    • :GoCoverage -> :GoCoverageBrowser
    • :GoCoverlay -> :GoCoverage
    • :GoClearlay -> :GoCoverageClear

Our current command will not open the browser anymore, instead it will use this current implementation. However if people want they can still access them via :GoCoverageBrowser

@fatih
Copy link
Owner

fatih commented Apr 3, 2016

Should we merge command mappings?

Yes, I've added notes. We're going to rename them and this implementation will replace the current command

Should we squash it?

I would prefer to squash it

It includes testsuite using VimFlavor. Should we remove it or use another test tool?

Right now we don't have any kinds of tests. But this is very high on my list soon. I have to make a research and then implement the tests. For now I think we can keep these tests and once we decide on the tests, I'll can change them.

It includes several ugly hacks to implement overlay. Colorize lines using matchadd

That's ok. But I was thinking of changing it. We should be able to highlight the whole line if possible. I'll look into it now.

No way to customize colors (requested in t-yuki/vim-go-coverlay#4 )

It's ok, I don't want people to change it. It's an implementation deteail and people should not change it. The same applies for go tool cover -html mode. There is green and red, and you can't change the color.

It doesn't redraw all open splits so non-active splits will not be colored until it gets the focus (reported in t-yuki/vim-go-coverlay#3 )

We can check this out, right now I'm not sure how this will affect but we probably can find a way

Excellent work btw, thank you very much including this :) I'm using coverage extensively, so I think this is a huge addition!

fatih
fatih reviewed Apr 3, 2016
View reviewed changes
autoload/go/coverlay.vim
autocmd!
autocmd BufEnter,BufWinEnter,BufFilePost * call go#coverlay#draw()
autocmd BufWinLeave * call go#coverlay#clear()
augroup END
Copy link
Owner

Choose a reason for hiding this comment

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

This should be moved to our autogroup section in file plugin/go.vim

@fatih
Copy link
Owner

fatih commented Apr 3, 2016

Alright I couldn't stop myself. I've made it faster by using matchaddpos and changed the way we display coverage. Checkout the gif:

screen recording 2016-04-03 at 11 11 pm

I'll merge your changes and then apply my patch (as I know what to do already). Let me finalize the latest bits on my part. Thanks again for the work, works really cool ;)

@fatih fatih mentioned this pull request Apr 3, 2016
Merged
@fatih fatih closed this Apr 3, 2016
@fatih
Copy link
Owner

fatih commented Apr 3, 2016

@t-yuki I've opened #786 with your commits squashed and included and my changes on top of it. I'm going to merge it. Thanks a lot for your work again! Please let me know if there is anything I can do.

@t-yuki
Copy link
Contributor Author

t-yuki commented Apr 4, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@t-yuki @fatih