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

Some improvements for :GoRun #1703

Closed
wants to merge 1 commit into from
Closed

Conversation

hiberabyss
Copy link
Contributor

@hiberabyss hiberabyss commented Mar 4, 2018

  1. Now will close previous :GoRun window when run more than one time;
  2. Fix bug when there is error after running :GoRun.

Bug Reproduce:

  1. Create directory /tmp/gotest
  2. Create file /tmp/gotest/test.go with following content:
package main

import "fmt"

func main() {
	abc
	greating := "hello, world!"
	fmt.Printf(greating)
}
  1. Run cd /tmp/gotest && nvim test.go;
  2. Run :GoRun;

Following result will occur:
image

The root cause is that s:on_exit close wrong window, since window is switched in go#term#newmode.

Thanks!

@codecov-io
Copy link

Codecov Report

Merging #1703 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1703      +/-   ##
==========================================
- Coverage   19.73%   19.69%   -0.04%     
==========================================
  Files          57       57              
  Lines        4748     4757       +9     
==========================================
  Hits          937      937              
- Misses       3811     3820       +9
Flag Coverage Δ
#nvim 15.05% <0%> (-0.03%) ⬇️
#vim74 17.25% <0%> (-0.04%) ⬇️
#vim80 18.62% <0%> (-0.04%) ⬇️
Impacted Files Coverage Δ
autoload/go/term.vim 0% <0%> (ø) ⬆️

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 8c6a186...6273805. Read the comment docs.

@hiberabyss
Copy link
Contributor Author

Hi,

Is there any suggestion for this PR?
Thanks!

Copy link
Collaborator

@bhcleek bhcleek left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

I am unable to duplicate the bug that you describe. Can you verify that you're seeing incorrect behavior using master? What value are you using for g:go_term_mode?

@hiberabyss
Copy link
Contributor Author

Hi @bhcleek ,

  1. Need to cd into the directory which contain the go file with error. This condition was missed in my previous bug reproduce description, I have updated it now.
  2. The bug could also be reproduced with the latest vim-go master branch (d2b0a23).
  3. My g:go_term_mode setting is botright split.
  4. My neovim version is v0.2.3-835-g739fb93a9. Different neovim version should have no compact.

Thanks!

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 19, 2018

With g:go_term_mode set to one of the options that starts with botright or rightbelow, I get a new location list every time :GoRun is executed. When g:go_term_mode is set to one of the options that starts with leftabove or topleft, I don't see any problems. I've documented what I discovered in #1720.

Interestingly, I can only get the terminal window shown in your screenshot if I comment out https://github.com/fatih/vim-go/blob/master/autoload/go/term.vim#L118.

While I agree there is a bug, I'm unable to duplicate the bug you're describing.

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 19, 2018

@hiberabyss Can you give #1721 a run and see if it resolves the issue you're seeing? I think the source of the bug you're seeing is a race condition that #1721 should resolve.

@hiberabyss
Copy link
Contributor Author

Hi @bhcleek ,

#1721 has resolved the issue, thanks for your great work!

Currently, when :GoRun N times, there will be N new windows. I expect there will be only one running window even when run :GoRun N times. Could this also be improved?

image

Thanks!

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 19, 2018

I'm going to look at improving that experience when I do #1412 after #1721 is merged

@bhcleek bhcleek closed this Mar 19, 2018
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