|
|
|
Created:
13 years, 4 months ago by Tobias Columbus Modified:
13 years, 4 months ago CC:
golang-dev, fss, dsymonds, minux1 Visibility:
Public. |
misc/vim: fix for autocompletion
Vim autocompletion respects the $GOPATH variable and does not
ignore dashes ('-'), dots ('.') and underscores ('_') like found
in many remote packages.
Environment variable $GOROOT is determined by calling
'go env GOROOT' instead of relying on the user's environment
variables.
Fixes issue 3876
Fixes issue 3882
Patch Set 1 #Patch Set 2 : diff -r ac1b735e8753 https://code.google.com/p/go/ #Patch Set 3 : diff -r ac1b735e8753 https://code.google.com/p/go/ #
Total comments: 2
Patch Set 4 : diff -r ac1b735e8753 https://code.google.com/p/go/ #
Total comments: 4
Patch Set 5 : diff -r ac1b735e8753 https://code.google.com/p/go/ #Patch Set 6 : diff -r ac1b735e8753 https://code.google.com/p/go/ #
Total comments: 2
Patch Set 7 : diff -r ac1b735e8753 https://code.google.com/p/go/ #Total messages: 21
|
Tobias Columbus
Hello golang-dev@googlegroups.com (cc: franciscossouza@gmail.com), I'd like you to review this change to https://code.google.com/p/go/
|
13 years, 4 months ago (2012年08月19日 20:02:08 UTC) #1 |
Hello golang-dev@googlegroups.com (cc: franciscossouza@gmail.com), I'd like you to review this change to https://code.google.com/p/go/
LGTM We can use `go env GOROOT` if the $GOROOT environment variable is not defined. +dsymonds https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... File misc/vim/autoload/go/complete.vim (right): https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:41: let dirs += [ goroot ] could we use `go env GOROOT` if $GOROOT environment variable is not defined? https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:44: if len( dirs ) == 0 if we use `go env GOROOT`, this condition is no longer needed.
I'm not so sure of the "go env GOROOT". It was there before my changes, so I left it. I know, that's no argument. But I think that not every user is happy, if vim clutters his environment without any notification. Perhaps it would be a better solution to either ask the user to set his $GOROOT variable or to enquire if $GOROOT should be set? tc On 2012年08月20日 13:59:54, fss wrote: > LGTM > > We can use `go env GOROOT` if the $GOROOT environment variable is not defined. > > +dsymonds > > https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... > File misc/vim/autoload/go/complete.vim (right): > > https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... > misc/vim/autoload/go/complete.vim:41: let dirs += [ goroot ] > could we use `go env GOROOT` if $GOROOT environment variable is not defined? > > https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... > misc/vim/autoload/go/complete.vim:44: if len( dirs ) == 0 > if we use `go env GOROOT`, this condition is no longer needed.
Okay, I was way too fast with my judgement. Apologies for that. I just tried "go help env" in my shell and realized that "go env GOROOT" returns the setting for $GOROOT instead of setting it. However, I still think, that this solution is not appropriate. If I execute export GOROOT= go env GOROOT then I get "/usr/local/go", which does not even exist. So, either my installation is broken or the "go env GOROOT" solution is not appropriate for a missing $GOROOT environment variable. Please correct me, if I'm wrong. tc On 2012年08月20日 14:16:35, Tobias Columbus wrote: > I'm not so sure of the "go env GOROOT". It was there before my changes, so I > left it. I know, that's no argument. > > But I think that not every user is happy, if vim clutters his environment > without any notification. > Perhaps it would be a better solution to either ask the user to set his $GOROOT > variable or to enquire if $GOROOT should be set? > > tc > > On 2012年08月20日 13:59:54, fss wrote: > > LGTM > > > > We can use `go env GOROOT` if the $GOROOT environment variable is not defined. > > > > +dsymonds > > > > > https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... > > File misc/vim/autoload/go/complete.vim (right): > > > > > https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... > > misc/vim/autoload/go/complete.vim:41: let dirs += [ goroot ] > > could we use `go env GOROOT` if $GOROOT environment variable is not defined? > > > > > https://codereview.appspot.com/6443151/diff/2002/misc/vim/autoload/go/complet... > > misc/vim/autoload/go/complete.vim:44: if len( dirs ) == 0 > > if we use `go env GOROOT`, this condition is no longer needed.
On Mon, Aug 20, 2012 at 11:10 PM, <tobias.columbus@googlemail.com> wrote: > However, I still think, that this solution is not appropriate. > If I execute > > export GOROOT= > go env GOROOT > > then I get "/usr/local/go", which does not even exist. > So, either my installation is broken or the "go env GOROOT" solution is > not appropriate for a missing $GOROOT environment variable. > the goroot is embedded in the binary of the go command, if you set $GOROOT to empty, it will display the internal one. imo, you don't need to worry about this problem, and just use the output of 'go env GOROOT'; as goroot is fairly important to the go command, if it's not correct, then you can't even compile go programs.
> On 2012年08月20日 16:02:31, minux wrote: > imo, you don't need to worry about this problem, and just use > the output of 'go env GOROOT'; as goroot is fairly important to > the go command, if it's not correct, then you can't even compile > go programs. +1 for minux suggestion. This change could close issues 3876 and 3882 automatically when it get's committed. To do that, run `hg change 6443151` and add "Fixes issue 3876." and "Fixes issue 3882." to the CL description.
Hello golang-dev@googlegroups.com, franciscossouza@gmail.com, dsymonds@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
This looks plausible to me. Just some style nits. I'm fine with this once minux has given a LGTM. minux, feel free to submit this at that point. http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:35: let goroot=matchstr(system('go env GOROOT'), '^\s*\f*') spaces either side of the "=" please. but why use matchstr at all? http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:40: let goroot=$GOROOT spaces around = http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:47: let workspaces = split( $GOPATH, ':' ) drop the spaces inside the parens http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:59: let root = expand(dir.'/pkg/'.s:goos.'_'.s:goarch) "." is a binary operator here, so add a space either side of them on this line. (yeah, it was wrong before too)
Tobias, please fill out the CLA: http://golang.org/doc/contribute.html#copyright
Hello golang-dev@googlegroups.com, franciscossouza@gmail.com, dsymonds@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
I hope I fixed all the style issues.
Let me explain the matchstr:
system('go env GOROOT') returns 'path\n'.
I came up with matchstr to get 'path' out of '\path\n'.
On 2012年08月21日 00:42:18, dsymonds wrote:
> This looks plausible to me. Just some style nits.
>
> I'm fine with this once minux has given a LGTM. minux, feel free to submit
this
> at that point.
>
>
http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete...
> File misc/vim/autoload/go/complete.vim (right):
>
>
http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete...
> misc/vim/autoload/go/complete.vim:35: let goroot=matchstr(system('go env
> GOROOT'), '^\s*\f*')
> spaces either side of the "=" please.
>
> but why use matchstr at all?
>
>
http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete...
> misc/vim/autoload/go/complete.vim:40: let goroot=$GOROOT
> spaces around =
>
>
http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete...
> misc/vim/autoload/go/complete.vim:47: let workspaces = split( $GOPATH, ':' )
> drop the spaces inside the parens
>
>
http://codereview.appspot.com/6443151/diff/9001/misc/vim/autoload/go/complete...
> misc/vim/autoload/go/complete.vim:59: let root =
> expand(dir.'/pkg/'.s:goos.'_'.s:goarch)
> "." is a binary operator here, so add a space either side of them on this
line.
>
> (yeah, it was wrong before too)
On Tue, Aug 21, 2012 at 10:48 AM, <tobias.columbus@googlemail.com> wrote: > Let me explain the matchstr: > system('go env GOROOT') returns 'path\n'. > I came up with matchstr to get 'path' out of '\path\n'. Maybe just trim space chars from the right instead.
On 2012年08月21日 00:52:56, dsymonds wrote:
> On Tue, Aug 21, 2012 at 10:48 AM, <mailto:tobias.columbus@googlemail.com>
wrote:
>
> > Let me explain the matchstr:
> > system('go env GOROOT') returns 'path\n'.
> > I came up with matchstr to get 'path' out of '\path\n'.
>
> Maybe just trim space chars from the right instead.
How would you do that?
The only other simple possibility I see in
:help function-list
is to substitute() newline characters.
I am no vim guru, so there may be better options. I'd like to hear one!
tc
On Tue, Aug 21, 2012 at 8:56 PM, <tobias.columbus@googlemail.com> wrote: > How would you do that? > The only other simple possibility I see in > :help function-list > is to substitute() newline characters. > > I am no vim guru, so there may be better options. I'd like to hear one! Yeah, substitute would be a bit clearer in intent.
Hello golang-dev@googlegroups.com, franciscossouza@gmail.com, dsymonds@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
LGTM with a small suggestion. @dsymonds, could you please add the author to C+A? http://codereview.appspot.com/6443151/diff/2006/misc/vim/plugin/godoc.vim File misc/vim/plugin/godoc.vim (right): http://codereview.appspot.com/6443151/diff/2006/misc/vim/plugin/godoc.vim#new... misc/vim/plugin/godoc.vim:75: let word = substitute(word, '[^a-zA-Z0-9\/._-]', '', 'g') how about add ~ here?
http://codereview.appspot.com/6443151/diff/2006/misc/vim/plugin/godoc.vim File misc/vim/plugin/godoc.vim (right): http://codereview.appspot.com/6443151/diff/2006/misc/vim/plugin/godoc.vim#new... misc/vim/plugin/godoc.vim:75: let word = substitute(word, '[^a-zA-Z0-9\/._-]', '', 'g') On 2012年08月23日 13:56:50, minux wrote: > how about add ~ here? Seems a good idea. At a second look, I think, one should also escape the backslash. I will fix it right now.
Hello golang-dev@googlegroups.com, franciscossouza@gmail.com, dsymonds@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
On Thu, Aug 23, 2012 at 11:56 PM, <minux.ma@gmail.com> wrote: > @dsymonds, could you please add the author to C+A? Sent: http://codereview.appspot.com/6480058. Feel free to clpatch it and submit it with this one.
On Fri, Aug 24, 2012 at 11:17 AM, David Symonds <dsymonds@golang.org> wrote: > On Thu, Aug 23, 2012 at 11:56 PM, <minux.ma@gmail.com> wrote: > >> @dsymonds, could you please add the author to C+A? > > Sent: http://codereview.appspot.com/6480058. Feel free to clpatch it > and submit it with this one. Actually I'll just submit it. minux, you're clear to submit this when you're ready.
*** Submitted as http://code.google.com/p/go/source/detail?r=7fb0e868dc39 *** misc/vim: fix for autocompletion Vim autocompletion respects the $GOPATH variable and does not ignore dashes ('-'), dots ('.') and underscores ('_') like found in many remote packages. Environment variable $GOROOT is determined by calling 'go env GOROOT' instead of relying on the user's environment variables. Fixes issue 3876 Fixes issue 3882 R=golang-dev, franciscossouza, dsymonds, minux.ma CC=golang-dev http://codereview.appspot.com/6443151 Committer: Shenghou Ma <minux.ma@gmail.com>