code review 14480043: oracle: Integration test for oracle.vim

oracle: Integration test for oracle.vim

Added a bash integration test for oracle.vim
Added some boilerplate to only load oracle.vim once

[rdallman10, 2013-10-07 03:45:18]

Hello adonovan@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[adonovan, 2013-10-07 18:25:42]

Thanks Reed, thanks for this change---looks like you found your way around the
code review system.

Lots of questions from me since I don't know Vimscript, I'm afraid.

cheers
alan

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/main.go
File cmd/oracle/main.go (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/main.go#newcode1
cmd/oracle/main.go:1: // Copyright 2013 The Go Authors. All rights reserved.
No changes here; remove this file from the CL.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim#newco...
cmd/oracle/oracle.vim:18: " load this plugin once
What problem does this solve?  (The equivalent code in Emacs would be
undesirable because it's common to load and re-load a script under development.)

Perhaps you want the test to say "vim -u /dev/null" to make it independent of
user configuration.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash
File cmd/oracle/vim-test.bash (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:7: set -e
set -eu

will catch more errors.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:30: vim -X -n \
Use:
  vim -u /dev/null
to avoid dependence on user configuration. 

Better still:

vim -X -n -u <(cat <<EOF
source oracle.vim
exec "/fmt"
GoOracleDescribe
redir @a
g/Sprintf/p
redir END
e $log
exec "normal \"ap\"
wqall
EOF)

or write it to an actual tmp file and use that.

Given how cryptic these commands are relative to Emacs', it would be good to
document what's going on. e.g.
# Runs a describe query at the "fmt" import, then ...
(I really have no idea what lines 34-39 do.)

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:31: -c "source oracle.vim" \
Shouldn't this be relative to $thisdir?

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:32: -c "exec \"/fmt\"" \
Do you really need exec?  /fmt should do.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:39: -c "wqall" \
Why are we saving anything?

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:40: main.go || die "vim command failed"
Shouldn't this also be relative to $thisdir?

[rdallman10, 2013-10-08 04:14:53]

Hello adonovan@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rdallman10, 2013-10-08 04:16:45]

think I covered most of the bases. hopefully my comments clear things up a bit.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim#newco...
cmd/oracle/oracle.vim:18: " load this plugin once
On 2013/10/07 18:25:43, adonovan wrote:
> What problem does this solve?  (The equivalent code in Emacs would be
> undesirable because it's common to load and re-load a script under
development.)
> 
> Perhaps you want the test to say "vim -u /dev/null" to make it independent of
> user configuration.

Honestly, great question. The best I can come up with is that it saves the load
time for vim on startup / config change. This boilerplate along with a "set
&cpo" I've seen in almost every vim plugin I've come across. VimL is awfully
slow, I guess I've just always deemed it good practice. 

I do see your point. Will remove if desired.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash
File cmd/oracle/vim-test.bash (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/vim-test.bash#ne...
cmd/oracle/vim-test.bash:39: -c "wqall" \
On 2013/10/07 18:25:43, adonovan wrote:
> Why are we saving anything?

to grep it in next step (I tried w/o). the buffer will be in $log at this point,
so I believe it would be the same as a >$log, effectively. "main.go" buffer
remains unmodified. Frankly, it yelled at me for having too many "-c" 's so I'm
trying to be frugal with the 'wqall'

[adonovan, 2013-10-10 19:09:58]

LGTM

Two little nitpicks.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim#newco...
cmd/oracle/oracle.vim:18: " load this plugin once
On 2013/10/08 04:16:45, rdallman10 wrote:
> On 2013/10/07 18:25:43, adonovan wrote:
> > What problem does this solve?  (The equivalent code in Emacs would be
> > undesirable because it's common to load and re-load a script under
> development.)
> > 
> > Perhaps you want the test to say "vim -u /dev/null" to make it independent
of
> > user configuration.
> 
> Honestly, great question. The best I can come up with is that it saves the
load
> time for vim on startup / config change. This boilerplate along with a "set
> &cpo" I've seen in almost every vim plugin I've come across. VimL is awfully
> slow, I guess I've just always deemed it good practice. 
> 
> I do see your point. Will remove if desired. 

Please do.  If we don't know we need it, we shouldn't include it.  If we don't
even know what it does or why, we especially shouldn't include it. :)

https://codereview.appspot.com/14480043/diff/15001/cmd/oracle/vim-test.bash
File cmd/oracle/vim-test.bash (right):

https://codereview.appspot.com/14480043/diff/15001/cmd/oracle/vim-test.bash#n...
cmd/oracle/vim-test.bash:29: # opens main.go and loads oracle.vim in
compatibility mode
Please punctuate as you would a college essay. :)
"Opens main.go, ..."
"... mode, finds ..."
etc.

https://codereview.appspot.com/14480043/diff/15001/cmd/oracle/vim-test.bash#n...
cmd/oracle/vim-test.bash:44: # previous command(s) should result in this
Redundant comment; delete.

[kisielk, 2013-10-10 21:11:01]

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim#newco...
cmd/oracle/oracle.vim:18: " load this plugin once
On 2013/10/10 19:09:58, adonovan wrote:
> On 2013/10/08 04:16:45, rdallman10 wrote:
> > On 2013/10/07 18:25:43, adonovan wrote:
> > > What problem does this solve?  (The equivalent code in Emacs would be
> > > undesirable because it's common to load and re-load a script under
> > development.)
> > > 
> > > Perhaps you want the test to say "vim -u /dev/null" to make it independent
> of
> > > user configuration.
> > 
> > Honestly, great question. The best I can come up with is that it saves the
> load
> > time for vim on startup / config change. This boilerplate along with a "set
> > &cpo" I've seen in almost every vim plugin I've come across. VimL is awfully
> > slow, I guess I've just always deemed it good practice. 
> > 
> > I do see your point. Will remove if desired. 
> 
> Please do.  If we don't know we need it, we shouldn't include it.  If we don't
> even know what it does or why, we especially shouldn't include it. :)

From :helpgrep NOT LOADING:

It's possible that a user doesn't always want to load this plugin.  Or the
system administrator has dropped it in the system-wide plugin directory, but a
user has his own plugin he wants to use.  Then the user must have a chance to
disable loading this specific plugin.  This will make it possible: >

  6     if exists("g:loaded_typecorr")
  7       finish
  8     endif
  9     let g:loaded_typecorr = 1

This also avoids that when the script is loaded twice it would cause error
messages for redefining functions and cause trouble for autocommands that are
added twice.

The name is recommended to start with "loaded_" and then the file name of the
plugin, literally.  The "g:" is prepended just to avoid mistakes when using
the variable in a function (without "g:" it would be a variable local to the
function).

[rdallman10, 2013-10-10 21:42:49]

I have added installation instructions. I added 2 options which I believe to be
the best, however there are other options. I'll expand on that. In the
go.tools/cmd/oracle we could add some sort of directory structure along the
lines of go.tools/cmd/oracle/vim/ftplugin/go.vim (if ftplugin is desired
behavior, which I think is correct in this instance). The user could then set
rtp+=.../go.tools/cmd/oracle/vim . I think the options I have provided are a
little cleaner on the go.tools side of things, but there is no right way to make
a PB&J. 

I left the g:loaded_oracle stuff... see kisielk comment for rationale. I would
still delete if such behavior is still unwanted. (not really meaning to be an
annoyance, I realize what a small bugaboo it is. it's just the "vim" way i've
seen in my use so far)

Also, I believe this "rudimentary" integration is at least sufficient to be of
use to Vim users at this point. Feel free to expand on the so called "usability
improvements" that are needed. I've found it pretty useful recently and don't
really have any complaints, but I'd be happy to work on said improvements.

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/14480043/diff/6001/cmd/oracle/oracle.vim#newco...
cmd/oracle/oracle.vim:18: " load this plugin once
On 2013/10/10 21:11:01, kisielk wrote:
> On 2013/10/10 19:09:58, adonovan wrote:
> > On 2013/10/08 04:16:45, rdallman10 wrote:
> > > On 2013/10/07 18:25:43, adonovan wrote:
> > > > What problem does this solve?  (The equivalent code in Emacs would be
> > > > undesirable because it's common to load and re-load a script under
> > > development.)
> > > > 
> > > > Perhaps you want the test to say "vim -u /dev/null" to make it
independent
> > of
> > > > user configuration.
> > > 
> > > Honestly, great question. The best I can come up with is that it saves the
> > load
> > > time for vim on startup / config change. This boilerplate along with a
"set
> > > &cpo" I've seen in almost every vim plugin I've come across. VimL is
awfully
> > > slow, I guess I've just always deemed it good practice. 
> > > 
> > > I do see your point. Will remove if desired. 
> > 
> > Please do.  If we don't know we need it, we shouldn't include it.  If we
don't
> > even know what it does or why, we especially shouldn't include it. :)
> 
> From :helpgrep NOT LOADING:
> 
> It's possible that a user doesn't always want to load this plugin.  Or the
> system administrator has dropped it in the system-wide plugin directory, but a
> user has his own plugin he wants to use.  Then the user must have a chance to
> disable loading this specific plugin.  This will make it possible: >
> 
>   6     if exists("g:loaded_typecorr")
>   7       finish
>   8     endif
>   9     let g:loaded_typecorr = 1
> 
> This also avoids that when the script is loaded twice it would cause error
> messages for redefining functions and cause trouble for autocommands that are
> added twice.
> 
> The name is recommended to start with "loaded_" and then the file name of the
> plugin, literally.  The "g:" is prepended just to avoid mistakes when using
> the variable in a function (without "g:" it would be a variable local to the
> function).

I found this url:
http://vim.1045645.n5.nabble.com/the-plugin-startup-check-td1200734.html which
offers a similar sentiment. Will let the debate roll on until further notice,
though I amended the variable to fit the naming convention.

[gobot, 2013-12-20 16:26:01]

Replacing golang-dev with golang-codereviews.

[gobot, 2014-12-19 05:13:24]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/14480043 is best) in the description in your
new CL.

Thanks very much.