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

Fix :GoBuild shell escaping #1450

Merged
merged 1 commit into from
Sep 14, 2017
Merged

Fix :GoBuild shell escaping #1450

merged 1 commit into from
Sep 14, 2017

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Sep 14, 2017

There was a stray go#util#Shelllist() in there. As near as I can tell
this isn't needed at all. Probably a left-over from some earlier
version?

Also move the non-async code in an else, I think this makes it a lot
clearer which code path does what. The two early returns were kinda
confusing.

Tested with Vim 8, 7.4, and Neovim, and seems to work well.

Fixes #1404

@fatih
Copy link
Owner

fatih commented Sep 14, 2017

At some point we should ditch supporting 7.4 Not sure when, probably not in near future but we should eventually.

lgtm

@arp242
Copy link
Contributor Author

arp242 commented Sep 14, 2017

At some point we should ditch supporting 7.4 Not sure when, probably not in near future but we should eventually.

I was thinking the same; Ubuntu 16.04 (the most recent LTS) has Vim 7.4.1689; the next LTS release (18.04) will have Vim 8, so that might be a good time?

I also saw that some functions don't actually have async support, for example for :GoInstall it's implemented for just Vim but not Neovim. It's annoying that the APIs are different :-/

@fatih
Copy link
Owner

fatih commented Sep 14, 2017

Yeah :/ The reason is that I'm not using neovim. Initially when it was released I've added some async capabilities for neovim. But then vim-go grew and grew,so I couldn't keep up with neovim support. I've decided after that vim-go should have first class support for Vim, but for Neovim it shouldn't have it. So whenever there was an issue for Neovim and I had time, I would add it. That's why the API is so different.

the next LTS release (18.04) will have Vim 8, so that might be a good time?

Yeap definitely!

There was a stray `go#util#Shelllist()` in there. As near as I can tell
this isn't needed at all. Probably a left-over from some earlier
version?

Also move the non-async code in an `else`, I think this makes it a lot
clearer which code path does what. The two early returns were kinda
confusing.

Tested with Vim 8, 7.4, and Neovim, and seems to work well.

Fixes #1404
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.

2 participants