code review 13656045: go.tools/cmd/oracle: Encoding issue, Possible to descri...

go.tools/cmd/oracle: Encoding issue, Possible to describe modified file
        line2byte doesn't handle non utf-8 fileencoding. So added s:getpos().
        And also, changing errorformat is not right way on go filetype.
        Added range operations.

[mattn, 2013-09-12 04:00:11]

Hello golang-dev@googlegroups.com,

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

[kisielk, 2013-09-12 04:49:45]

https://codereview.appspot.com/13656045/diff/11001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/13656045/diff/11001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:21: let go_oracle = globpath($GOPATH, '/bin/oracle' . ext)
This isn't reliable because $GOPATH may contain multiple paths separated by a
colon. 

Other vim plugins included in misc/vim just rely on the binary being within
$PATH. For example
https://code.google.com/p/go/source/browse/misc/vim/plugin/godoc.vim#73

[mattn, 2013-09-12 05:25:59]

https://codereview.appspot.com/13656045/diff/11001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/13656045/diff/11001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:21: let go_oracle = globpath($GOPATH, '/bin/oracle' . ext)
globpath treat the colon, and return first found.

On 2013/09/12 04:49:45, kisielk wrote:
> This isn't reliable because $GOPATH may contain multiple paths separated by a
> colon. 
> 
> Other vim plugins included in misc/vim just rely on the binary being within
> $PATH. For example
> https://code.google.com/p/go/source/browse/misc/vim/plugin/godoc.vim#73

[mattn, 2013-09-12 05:30:33]

https://codereview.appspot.com/13656045/diff/11001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/13656045/diff/11001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:21: let go_oracle = globpath($GOPATH, '/bin/oracle' . ext)
On 2013/09/12 05:25:59, mattn wrote:
> globpath treat the colon, and return first found.
> 
> On 2013/09/12 04:49:45, kisielk wrote:
> > This isn't reliable because $GOPATH may contain multiple paths separated by
a
> > colon. 
> > 
> > Other vim plugins included in misc/vim just rely on the binary being within
> > $PATH. For example
> > https://code.google.com/p/go/source/browse/misc/vim/plugin/godoc.vim#73
> 

Ah, sorry. globpath doesn't treat colon. It's a comma. I'll fix.

[mattn, 2013-09-12 05:52:52]

Done.

And I didn't remove the part go_oracle_bin() that find executable. 'oracle' is
too generaly name for me.

[kisielk, 2013-09-12 18:06:47]

LGTM but probably someone with more vimscript experience should look at it too.

[mattn, 2013-09-14 16:13:36]

dsymonds, could you please review this?

On Friday, September 13, 2013 3:06:47 AM UTC+9, Kamil Kisiel wrote:
>
> LGTM but probably someone with more vimscript experience should look at 
> it too. 
>
> https://codereview.appspot.com/13656045/ 
>

[mattn, 2013-09-17 07:11:05]

Added dsymonds as reviewer. Could you take a time to review?

[dsymonds, 2013-09-17 07:16:32]

I haven't looked too closely, and haven't played with the oracle yet myself, but
this looks plausible. Just a couple of questions/notes.

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:23: let go_oracle = globpath($GOROOT, '/bin/oracle' . ext)
can you invoke this as "go tool oracle" instead? That'd be more portable I
think.

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:57: if &encoding != 'utf-8'
I don't get why you're even trying to handle non-UTF-8 source. Go is defined as
UTF-8.

[Dominik Honnef, 2013-09-17 07:24:40]

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:23: let go_oracle = globpath($GOROOT, '/bin/oracle' . ext)
On 2013/09/17 07:16:32, dsymonds wrote:
> can you invoke this as "go tool oracle" instead? That'd be more portable I
> think.

Not yet. go tool doesn't know about the oracle yet.

[mattn, 2013-09-17 07:33:14]

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim
File cmd/oracle/oracle.vim (right):

https://codereview.appspot.com/13656045/diff/25001/cmd/oracle/oracle.vim#newc...
cmd/oracle/oracle.vim:57: if &encoding != 'utf-8'
On 2013/09/17 07:16:32, dsymonds wrote:
> I don't get why you're even trying to handle non-UTF-8 source. Go is defined
as
> UTF-8.

I'm using windows with encoding=cp932 & fileencoding=utf-8.

[dsymonds, 2013-09-18 04:01:46]

LGTM

okay, seems good enough for now I guess.

[mattn, 2013-09-20 00:30:58]

On 2013/09/18 04:01:46, dsymonds wrote:
> LGTM
> 
> okay, seems good enough for now I guess.

Who can submit this?

[dsymonds, 2013-09-20 01:09:34]

*** Submitted as
https://code.google.com/p/go/source/detail?r=3211db8ff86e&repo=tools ***

go.tools/cmd/oracle: Encoding issue, Possible to describe modified file
        line2byte doesn't handle non utf-8 fileencoding. So added s:getpos().
        And also, changing errorformat is not right way on go filetype.
        Added range operations.

R=golang-dev, kamil.kisiel, dsymonds, dominik.honnef
CC=golang-dev
https://codereview.appspot.com/13656045

Committer: David Symonds <dsymonds@golang.org>