fix missing <buffer> for debug mappings

[Jagua]

No description provided.

[codecov-io]

Codecov Report

Merging #1696 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1696      +/-   ##
==========================================
+ Coverage    19.7%   19.72%   +0.01%     
==========================================
  Files          57       57              
  Lines        4735     4746      +11     
==========================================
+ Hits          933      936       +3     
- Misses       3802     3810       +8

Flag	Coverage Δ
#nvim	15.06% <0%> (-0.04%)	⬇️
#vim74	17.27% <0%> (-0.05%)	⬇️
#vim80	18.64% <0%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
autoload/go/debug.vim	0% <0%> (ø)	⬆️
autoload/go/lint.vim	57.3% <0%> (+0.73%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f9c2e9...e757eda. Read the comment docs.

[arp242]

Thanks for your PR!

Could you explain what issue this fixes? Right now I can do:

vim a.go
:GoDebugStart
<F9>

:e b.go
<F9>
<F5>

To place a breakpoint in both a.go and b.go, which I think is useful. But with this PR I won't be able to do that any more, since the mappings are just for the a.go buffer.

Does this fix a specific error or issue? If so, then we should probably fix that, instead of making the mappings buffer-local.

[Jagua]

I have fixed that issue since my first PR was incomplete.

I think that a plugin should not override GLOBAL mappings.
That is, once executing :GoDebugStart, valuable keymaps such as <F6> will remain broken when editing for other languages.

This PR fixes this issue.

[arp242]

Thanks. The issue with this is that the mappings are, well, only available in the buffer. For example with :tabedit foo.go they won't be available. Personally I think it's useful to have that work. Same with some other cases.

I think a better fix is to not map stuff if it's already mapped:

if !hasmapto('<Plug>(go-debug-continue)')
  nmap <F5>   <Plug>(go-debug-continue)
endif
if !hasmapto('<Plug>('(go-debug-print)')
  nmap <F6>   <Plug>(go-debug-print)
endif

" ..etc..

Or only map the F-keys if it's not mapped to something else already:

if mapcheck('<F5>') is ''
  nmap <F5>   <Plug>(go-debug-continue)
endif

We could perhaps also add an option:

if get(g:, 'go_debug_map', 1) is 0
  nmap <F5>   <Plug>(go-debug-continue)
endif

You can then add let g:go_debug_map to disable the default mappings.

I don't mind making a patch, but can't really decide which option is best here.

[fatih]

I don't mind making a patch, but can't really decide which option is best here.

Making them optional with <Plug> is the correct way. That's the way to remap mappings and we already have a dedicated section of mappings and started using them since the project has started. But I think we should hold it up to the next release. It's not urgent imho.

[Jagua]

For example with :tabedit foo.go they won't be available.

Hmm, my patch (second commit) works fine in my environment.
(Because, mappings are defined by autocmd in each buffer which filetype is "go".)

However, if it does not work properly, I believe it is a good idea to prepare a variable such like g:go_debug_map . Thanks.

[arp242]

Hm, maybe I tested it wrong, but when I tried it, it didn't seem to work. I will look again later.

[arp242]

Okay, does seem to work. Dunno what I did wrong previously.

Should still make it configurable in the future. I'll make an issue for that, but this improves things so I'll merge it as well. Thanks 👍