improve neovim support

[hiberabyss]

1. Since a:mode will always be "", it's better to use mode. When user set let g:go_term_mode = "split", it could be supported.
2. Use =~ instead == to cover case like botright split.
3. Window switch should be done after all terminal buffer command finished.

[bhcleek]

matching on a regular expression here would change something like left vertical to vertical resize. Is that what you intended?

[hiberabyss]

No, I'm not quite sure how it works when set mode as vertical. In fact, when I set let g:go_term_mode = "vertical", following error occured:

It should be something like vsplit?

Thanks!

[hiberabyss]

Hi @bhcleek , do you have any suggestion on this pull request?

Thanks!

[bhcleek]

Thanks for contributing @hiberabyss!

I'm not sure what problem it's trying to solve, but I have a few concerns.

Since a:mode will always be "", it's better to use mode. When user set let g:go_term_mode = "split", it could be supported.

It may be worth noting that a:mode isn't always the empty string; it's passed in as the empty string when called from go#Run#Term in autoload/go/cmd.vim, but g:go_term_mode is used when called from go#term#new (via go#test#Test in autoload/go/test.vim). I'm not sure if that changes your assessment of whether mode should be used instead of a:mode. Note that mode may be g:go_term_mode while a:mode is the empty string...

Use =~ instead == to cover case like botright split.

I think this may be reasonable, but we still have some gaps in behavior. For instance, if g:go_term_mode is botright, then the correct resizing won't be done.

Window switch should be done after all terminal buffer command finished.
I don't see a problem with this on its face, but I'm curious; was the previous location of the window switch causing a problem?

I think an important piece of this puzzle is that b3a9f8b seems to have changed execute mode.' new' to execute mode.' __go_term__'. I suspect that was a mistake; the terminal window should be in a new window, not an existing window. If I'm correct, then that would also imply

• the window switch may need to be left where it was.
• the only supported values for g:go_term_mode are split and vertical. Adding support for the other split modifiers (e.g. botright, leftabove, et. al.) would be a nice addition, but won't be able to be accomplished easily with a regular expression.

[hiberabyss]

Hi @bhcleek ,

For your concerns:

1. The go#term#new used by go#test#Test in autoload/go/test.vim use g:go_term_mode as parameter, which means my change has no impact for this case. Since mode is same as a:mode.

However, when run in neovim, the a:mode will always be empty, which means the resize setting could not take effect if use a:mode instead of mode.

Moreover, the code will looks a little strange if some place use mode, while use a:mode in other place in same function.

It may be worth noting that a:mode isn't always the empty string; it's passed in as the empty string when called from go#Run#Term in autoload/go/cmd.vim, but g:go_term_mode is used when called from go#term#new (via go#test#Test in autoload/go/test.vim). I'm not sure if that changes your assessment of whether mode should be used instead of a:mode. Note that mode may be g:go_term_mode while a:mode is the empty string...

2. It make sense to use execute mode.' new' instead of execute mode.' __go_term__', I will push a new commit to modify this.

When use set leftabove vertical, he should also expect to resize the window width.

Currently, use regex match won't introduce bad impact, while it could cover more use cases. I think it deservers the change.

• the window switch may need to be left where it was.
• the only supported values for g:go_term_mode are split and vertical. Adding support for the other split modifiers (e.g. botright, leftabove, et. al.) would be a nice addition, but won't be able to be accomplished easily with a regular expression.

3. If keep following code in current place, wrong window will be resized:

  if l:winnr !=# winnr()
    exe l:winnr . "wincmd w"
  endif

Since let l:winnr = winnr() is executed before new terminal window created, its value is code window number. If run the switch command before resize command, the code window will be resized, which is not expected.

[codecov-io]

Codecov Report

Merging #1611 into master will increase coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1611      +/-   ##
==========================================
+ Coverage   19.15%   19.19%   +0.03%     
==========================================
  Files          53       53              
  Lines        4151     4153       +2     
==========================================
+ Hits          795      797       +2     
  Misses       3356     3356

Flag	Coverage Δ
#nvim	14.54% <0%> (+0.04%)	⬆️
#vim74	17.57% <0%> (+0.03%)	⬆️
#vim80	18.34% <0%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
autoload/go/term.vim	0% <0%> (ø)	⬆️
autoload/go/template.vim	64.7% <0%> (+2.2%)	⬆️

---

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 f4b4ba1...d660916. Read the comment docs.

[bhcleek]

resizing

You're right that when g:go_term_mode is vertical split that it will be detected, but it's entirely possible that g:go_term_mode is merely leftabove: in that case, vim-go would do a horizontal split, but would not resize the window correctly. If vim-go is going to support more than just vertical and split, then it should support the other valid options equally well. Supported values should handle strings that contain vsplit, split, vertical, and all the other window position commands: http://vimhelp.appspot.com/windows.txt.html#%3Avertical.

opening the window

Thanks for adding the new back in, but after working with it, I think that perhaps it was correct before, but that g:go_term_mode needs a default (e.g. get(g:, 'go_term_mode', 'split') that gets applied in the case that a:mode is empty (https://github.com/fatih/vim-go/blob/master/autoload/go/term.vim#L16-L19), because if g:go_term_mode is set to split or vsplit, then the terminal window now ends up being named new __go_term__.

[hiberabyss]

Hi @bhcleek ,

From the vim-go document, we could know that g:go_term_mode should be something like split , vsplit, botright split , leftabove vsplit and so on.

                                                       *'g:go_term_mode'*

This option is Neovim only. Use it to change the default command used to
open a new terminal for go commands such as |:GoRun|.
The default is vsplit.

let g:go_term_mode = "vsplit"

It should be meaningless to set g:go_term_mode as leftabove or vertical, since these command alone won't create a new window.

I think maybe we could change vertical to vsplit. Is it OK for you?

Thanks!

[hiberabyss]

Hi @bhcleek , is there any update?

[bhcleek]

This still needs:

• a default value in case g:go_term_mode is not set. https://github.com/fatih/vim-go/pull/1611/files#diff-2414f9dfad52aa87fa2afaf8d30a9d30R18 should be let mode = get( g:, 'go_term_mode', 'new')
• do the vertical resizing if the l:mode contains either vsplit or vertical.
• do horizontal resizing otherwise.

[hiberabyss]

@bhcleek ,

1. The default value of g:go_term_mode has already been set at beginning of the script with following code:

if has('nvim') && !exists("g:go_term_mode")
  let g:go_term_mode = 'vsplit'
endif

2. The code check whether mode contains vertical has already been added.

[bhcleek]

if mode =~ "split"
      exe 'resize ' . height	
elseif mode =~ "vsplit" || mode =~ "vertical"
      exe 'vertical resize ' . width
endif

Will resize the height instead of the width when mode is vsplit, because =~ "split" which is checked first.

[hiberabyss]

Hi @bhcleek ,

It makes sense. Thanks for your reminding!
I have updated the code.

[bhcleek]

Thanks @hiberabyss