Issue 4837051: code review 4837051: misc/vim: command complete using autoload helper function. (Closed)
|
Created:
12 years, 9 months ago by mattn Modified:
12 years, 8 months ago Reviewers:
CC:
golang-dev_googlecode.com, dsymonds, jnw, niemeyer, gustavo_niemeyer.net, golang-dev Visibility:
Public. |
Descriptionmisc/vim: command complete using autoload helper function.
Patch Set 1 #Patch Set 2 : diff -r 2ffd085d488a http://go.googlecode.com/hg/ #Patch Set 3 : diff -r 2ffd085d488a http://go.googlecode.com/hg/ #Patch Set 4 : diff -r 2ffd085d488a http://go.googlecode.com/hg/ #Patch Set 5 : diff -r 2ffd085d488a http://go.googlecode.com/hg/ #Patch Set 6 : diff -r 2ffd085d488a http://go.googlecode.com/hg/ #Patch Set 7 : diff -r 2ffd085d488a http://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 8 : diff -r 06908b66d618 http://go.googlecode.com/hg/ #
Total comments: 10
Patch Set 9 : diff -r d499fb951a9e http://go.googlecode.com/hg/ #
Total comments: 5
Patch Set 10 : diff -r daa11382c852 http://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 11 : diff -r daa11382c852 http://go.googlecode.com/hg/ #Patch Set 12 : diff -r daa11382c852 http://go.googlecode.com/hg/ #Patch Set 13 : diff -r daa11382c852 http://go.googlecode.com/hg/ #
MessagesTotal messages: 21
Hello golang-dev@googlecode.com, dsymonds@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to http://go.googlecode.com/hg/
Sign in to reply to this message.
Hello golang-dev@googlecode.com, dsymonds@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I'll review this tomorrow. In the meantime, could you explain the reason for this change?
Sign in to reply to this message.
On Wed, Aug 3, 2011 at 5:54 PM, mattn <mattn.jp@gmail.com> wrote: > Completing name of package for Import/ImportAs command is useful i guess. > But It shouldn't write a copy of complete function in godoc.vim. And more, > autoload make help to be memory-saving. Okay, that makes sense. > BTW, I think that readme.txt should be only below. I disagree. Playing with rtp is fine for individual users to take on themselves, but it's too easy to get confused by it when things go wrong. I'd prefer to leave things the way we've got them, where people can cherry-pick the add-ons that they care about. Also, the lowest common denominator Go programmer won't necessarily have a full $GOROOT; they may very well have only the binaries and package files (e.g. installed from a Debian package or something). The misc/vim stuff shouldn't be dependent on $GOROOT. Dave.
Sign in to reply to this message.
I'd like some other Vim users to chime in on this change too. http://codereview.appspot.com/4837051/diff/5004/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/5004/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:29: endif Can you move everything up to this line into a once-run function? http://codereview.appspot.com/4837051/diff/5004/misc/vim/readme.txt File misc/vim/readme.txt (right): http://codereview.appspot.com/4837051/diff/5004/misc/vim/readme.txt#newcode4 misc/vim/readme.txt:4: To install some plugins below, add following line into your vimrc. "To use all the Vim plugins, add these files to your vimrc." http://codereview.appspot.com/4837051/diff/5004/misc/vim/readme.txt#newcode10 misc/vim/readme.txt:10: If choice to install plugin you want, read following notes. "If you want to select fewer plugins, use the instructions in the rest of this file."
Sign in to reply to this message.
Thanks your reviews. http://codereview.appspot.com/4837051/diff/5004/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/5004/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:29: endif On 2011/08/05 05:52:02, dsymonds wrote: > Can you move everything up to this line into a once-run function? Done. http://codereview.appspot.com/4837051/diff/5004/misc/vim/readme.txt File misc/vim/readme.txt (right): http://codereview.appspot.com/4837051/diff/5004/misc/vim/readme.txt#newcode4 misc/vim/readme.txt:4: To install some plugins below, add following line into your vimrc. On 2011/08/05 05:52:02, dsymonds wrote: > "To use all the Vim plugins, add these files to your vimrc." Done. http://codereview.appspot.com/4837051/diff/5004/misc/vim/readme.txt#newcode10 misc/vim/readme.txt:10: If choice to install plugin you want, read following notes. On 2011/08/05 05:52:02, dsymonds wrote: > "If you want to select fewer plugins, use the instructions in the rest of this > file." Done.
Sign in to reply to this message.
+ a couple of people who have reviewed previous misc/vim changes On Fri, Aug 5, 2011 at 3:52 PM, <dsymonds@golang.org> wrote: > I'd like some other Vim users to chime in on this change too. Guys, do you have feedback on this change? Dave.
Sign in to reply to this message.
On 2011/08/09 05:15:57, dsymonds wrote: > + a couple of people who have reviewed previous misc/vim changes > > On Fri, Aug 5, 2011 at 3:52 PM, <mailto:dsymonds@golang.org> wrote: > > I'd like some other Vim users to chime in on this change too. > > Guys, do you have feedback on this change? I think in general we need to do the following: 1. Write a help file so :help go or :help golang can detail the various things we allow here. It's just good vim practice. 2. Make some of the features of this bundle configurable using variables, for example documenting a way for the user to specify the pkg directory for searching, goos, etc. Then we can simplify the installation instructions and just allow them to turn features on and off at will. Neither of those is necessarily should be under the scope of this change. I'd like to see the comments in go/complete.vim updated to be more descriptive, but this all seems to make sense to me.
Sign in to reply to this message.
I'm in a sprint this week, so can't provide a very detailed view right now, but from a high-level perspective it feels like this is going in a nice direction. Here are a few comments: http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:30: " should not occur error. Why not? It feels quite reasonable that people would be editing a Go file without $GOROOT set. There are ways we can detect $GOROOT, but it doesn't have to be supported right now. Even then, "Can't handle it right now" is a more accurate statement than "shouldn't happen". http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:34: let root = expand($GOROOT.'/pkg/'.s:goos.'_'.s:goarch) What about $GOPATH? http://codereview.appspot.com/4837051/diff/6002/misc/vim/ftplugin/go/import.vim File misc/vim/ftplugin/go/import.vim (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/ftplugin/go/import.v... misc/vim/ftplugin/go/import.vim:39: command! -buffer -nargs=? -complete=customlist,go#complete#Package Drop call s:SwitchImport(0, '', <f-args>) This should complete from the imported packages, not from the tree. http://codereview.appspot.com/4837051/diff/6002/misc/vim/readme.txt File misc/vim/readme.txt (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/readme.txt#newcode6 misc/vim/readme.txt:6: set rtp+=$GOROOT/misc/vim I'd use runtimepath here, rather than rtp. Gives people a better sense of what they're doing.
Sign in to reply to this message.
Sorry, forgot to send this this morning. http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:3: " license that can be found in the LICENSE file. Please describe what this file does and contains in comments following the copyright notice. We're missing this in a few other files, but we have it in indent/go and we should get better at making sure it's included.
Sign in to reply to this message.
http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:3: " license that can be found in the LICENSE file. Sorry. I'm not native speaker. So I can't understand your meanings. What do you mean? * This file should add comment of "indent/go uses this autoload functions"? Or * This file is included from indent/go ? On 2011/08/09 18:10:07, jnw wrote: > Please describe what this file does and contains in comments following the > copyright notice. We're missing this in a few other files, but we have it in > indent/go and we should get better at making sure it's included. http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:30: " should not occur error. I guess this have better to be graceful failure. It make screen break while editing command line. (But this is vim's problem) On 2011/08/09 17:48:08, niemeyer wrote: > Why not? It feels quite reasonable that people would be editing a Go file > without $GOROOT set. There are ways we can detect $GOROOT, but it doesn't have > to be supported right now. Even then, "Can't handle it right now" is a more > accurate statement than "shouldn't happen". http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:34: let root = expand($GOROOT.'/pkg/'.s:goos.'_'.s:goarch) Do you mean that we have better to add $GOPATH to specify whole path sting to package? On 2011/08/09 17:48:08, niemeyer wrote: > What about $GOPATH?
Sign in to reply to this message.
> I guess this have better to be graceful failure. > It make screen break while editing command line. (But this is vim's > problem) Agreed, vim shouldn't break in awkward ways if $GOROOT isn't set. > Do you mean that we have better to add $GOPATH to specify whole path > sting to package? No, GOPATH allows providing alternative lookup paths for packages. Check this out: http://golang.org/cmd/goinstall It doesn't feel like this CL should be blocked on this, though. It's a simple change for a follow up CL. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
On Wed, Aug 10, 2011 at 2:18 PM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> Do you mean that we have better to add $GOPATH to specify whole path >> sting to package? > > No, GOPATH allows providing alternative lookup paths for packages. > > Check this out: http://golang.org/cmd/goinstall > > It doesn't feel like this CL should be blocked on this, though. It's > a simple change for a follow up CL. Yes, let's please punt on GOPATH for now. Dave.
Sign in to reply to this message.
http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:3: " license that can be found in the LICENSE file. On 2011/08/10 01:12:28, mattn wrote: > Sorry. I'm not native speaker. So I can't understand your meanings. What do you > mean? > > * This file should add comment of "indent/go uses this autoload functions"? > > Or > > * This file is included from indent/go ? > > > On 2011/08/09 18:10:07, jnw wrote: > > Please describe what this file does and contains in comments following the > > copyright notice. We're missing this in a few other files, but we have it in > > indent/go and we should get better at making sure it's included. > Something like "This file provides a utility function that performs auto-completion of package names, for use by other commands." would be more than suffficient!
Sign in to reply to this message.
http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... misc/vim/autoload/go/complete.vim:3: " license that can be found in the LICENSE file. On 2011/08/10 07:04:36, jnw wrote: > On 2011/08/10 01:12:28, mattn wrote: > > Sorry. I'm not native speaker. So I can't understand your meanings. What do > you > > mean? > > > > * This file should add comment of "indent/go uses this autoload functions"? > > > > Or > > > > * This file is included from indent/go ? > > > > > > On 2011/08/09 18:10:07, jnw wrote: > > > Please describe what this file does and contains in comments following the > > > copyright notice. We're missing this in a few other files, but we have it in > > > indent/go and we should get better at making sure it's included. > > > > Something like "This file provides a utility function that performs > auto-completion of package names, for use by other commands." would be more than > suffficient! Done.
Sign in to reply to this message.
On 2011/08/10 07:28:43, mattn wrote: > http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... > File misc/vim/autoload/go/complete.vim (right): > > http://codereview.appspot.com/4837051/diff/6002/misc/vim/autoload/go/complete... > misc/vim/autoload/go/complete.vim:3: " license that can be found in the LICENSE > file. > On 2011/08/10 07:04:36, jnw wrote: > > On 2011/08/10 01:12:28, mattn wrote: > > > Sorry. I'm not native speaker. So I can't understand your meanings. What do > > you > > > mean? > > > > > > * This file should add comment of "indent/go uses this autoload functions"? > > > > > > Or > > > > > > * This file is included from indent/go ? > > > > > > > > > On 2011/08/09 18:10:07, jnw wrote: > > > > Please describe what this file does and contains in comments following the > > > > copyright notice. We're missing this in a few other files, but we have it > in > > > > indent/go and we should get better at making sure it's included. > > > > > > > Something like "This file provides a utility function that performs > > auto-completion of package names, for use by other commands." would be more > than > > suffficient! > > Done. LGTM from my side of things.
Sign in to reply to this message.
LGTM fix these nits, and I'll submit. http://codereview.appspot.com/4837051/diff/15003/misc/vim/autoload/go/complet... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/15003/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:33: " should not occur error. " should not occur. http://codereview.appspot.com/4837051/diff/15003/misc/vim/readme.txt File misc/vim/readme.txt (right): http://codereview.appspot.com/4837051/diff/15003/misc/vim/readme.txt#newcode4 misc/vim/readme.txt:4: To use all the Vim plugins, add these files to your vimrc. s/files/lines/
Sign in to reply to this message.
I'm not seeing the changes from my last set of comments. http://codereview.appspot.com/4837051/diff/15003/misc/vim/autoload/go/complet... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/15003/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:33: " should not occur error. On 2011/08/16 00:32:29, dsymonds wrote: > " should not occur. no, I meant that this comment should read "should not occur." http://codereview.appspot.com/4837051/diff/24002/misc/vim/autoload/go/complet... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/24002/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:33: let goroot = $GOPATH drop GOPATH for now.
Sign in to reply to this message.
http://codereview.appspot.com/4837051/diff/15003/misc/vim/autoload/go/complet... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/15003/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:33: " should not occur error. On 2011/08/16 00:32:29, dsymonds wrote: > " should not occur. Done. http://codereview.appspot.com/4837051/diff/15003/misc/vim/readme.txt File misc/vim/readme.txt (right): http://codereview.appspot.com/4837051/diff/15003/misc/vim/readme.txt#newcode4 misc/vim/readme.txt:4: To use all the Vim plugins, add these files to your vimrc. On 2011/08/16 00:32:29, dsymonds wrote: > s/files/lines/ Done. http://codereview.appspot.com/4837051/diff/24002/misc/vim/autoload/go/complet... File misc/vim/autoload/go/complete.vim (right): http://codereview.appspot.com/4837051/diff/24002/misc/vim/autoload/go/complet... misc/vim/autoload/go/complete.vim:33: let goroot = $GOPATH On 2011/08/18 01:33:54, dsymonds wrote: > drop GOPATH for now. Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=1ce42d48298d *** misc/vim: command complete using autoload helper function. R=golang-dev, dsymonds, jnwhiteh, n13m3y3r, gustavo CC=golang-dev http://codereview.appspot.com/4837051 Committer: David Symonds <dsymonds@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
