code review 10564043: misc/vim: Handle import paths with periods in Godoc

misc/vim: Handle import paths with periods in Godoc

This patch makes the Godoc Vim plugin correctly split package
names with periods in them.

Fixes issue 5656.

[glts, 2013-06-25 19:53:03]

Hello golang-dev@googlegroups.com (cc: kamil.kisiel@gmail.com),

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

[kisielk, 2013-06-25 21:16:40]

LGTM, tested it out in a variety of ways and it seems to work.

Should get dsymonds to review since he's the Vim guy.

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim
File misc/vim/plugin/godoc.vim (right):

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
misc/vim/plugin/godoc.vim:12: "       Open the relevant Godoc for either the
word[s] passed to the command or
Unrelated to your change but I noticed the grammar for this description is
messed up.

[kisielk, 2013-06-25 23:21:13]

I actually found one more case that's not covered here:

If you have an import statement with a path that contains periods and use :Godoc
while over that it still interprets the first component of the path before the
period as the package and the rest of it as the member of the package.

[gobot, 2013-06-26 05:43:15]

R=dsymonds (assigned by dsymonds)

[dsymonds, 2013-06-26 05:45:48]

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim
File misc/vim/plugin/godoc.vim (right):

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
misc/vim/plugin/godoc.vim:12: "       Open the relevant Godoc for either the
word[s] passed to the command or
On 2013/06/25 21:16:40, kisielk wrote:
> Unrelated to your change but I noticed the grammar for this description is
> messed up.

yeah, could you fix this up while you're here?

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
misc/vim/plugin/godoc.vim:103: let words = split(word, '\.\ze[^/]*$')
This seems to implement the right thing, but in a roundabout way. I believe what
we want is to split words like "http.Request" into ["http", "Request"], and this
change is trying to cope with "github.com/blah/x.Whatever", right? So I think
the right approach is to instead find the final dot, and split into two pieces
on that, which would be '\.\ze[^.]*$'?

[glts, 2013-06-26 19:40:53]

On 2013/06/25 23:21:13, kisielk wrote:
> I actually found one more case that's not covered here:
> 
> If you have an import statement with a path that contains periods and use
:Godoc
> while over that it still interprets the first component of the path before the
> period as the package and the rest of it as the member of the package.

Yes, let me fix that too.

[glts, 2013-06-26 19:44:58]

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim
File misc/vim/plugin/godoc.vim (right):

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
misc/vim/plugin/godoc.vim:12: "       Open the relevant Godoc for either the
word[s] passed to the command or
On 2013/06/26 05:45:49, dsymonds wrote:
> On 2013/06/25 21:16:40, kisielk wrote:
> > Unrelated to your change but I noticed the grammar for this description is
> > messed up.
> 
> yeah, could you fix this up while you're here?

Will do.

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
misc/vim/plugin/godoc.vim:103: let words = split(word, '\.\ze[^/]*$')
On 2013/06/26 05:45:49, dsymonds wrote:
> This seems to implement the right thing, but in a roundabout way. I believe
what
> we want is to split words like "http.Request" into ["http", "Request"], and
this
> change is trying to cope with "github.com/blah/x.Whatever", right? So I think
> the right approach is to instead find the final dot, and split into two pieces
> on that, which would be '\.\ze[^.]*$'?

I too would be curious to see a more straightforward solution. We have
to cover all possible cases though:
strings
strings.Join
crypto/md5
crypto/md5.Size
pkg.me.com/what/is
pkg.me.com/what/is.There
I used the tersest (yet idiomatic) solution I could come up with that
covers all of them.

Note that there are ambiguous cases. Take "me.Com": does it refer to the
documentation for the package "me.Com" or for the exported member "Com"
of package "me"? But this is a problem we should address when we fix the
flawed interface of :Godoc. It has other problems too, like accepting
multiple arguments even though it can't handle them. Can we do this in a
separate issue?

[glts, 2013-06-26 19:46:11]

Hello dsymonds@golang.org (cc: dsymonds@golang.org, gobot@golang.org,
golang-dev@googlegroups.com, kamil.kisiel@gmail.com),

Please take another look.

[dsymonds, 2013-06-27 01:33:19]

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim
File misc/vim/plugin/godoc.vim (right):

https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
misc/vim/plugin/godoc.vim:103: let words = split(word, '\.\ze[^/]*$')
On 2013/06/26 19:44:58, glts wrote:
> On 2013/06/26 05:45:49, dsymonds wrote:
> > This seems to implement the right thing, but in a roundabout way. I believe
> what
> > we want is to split words like "http.Request" into ["http", "Request"], and
> this
> > change is trying to cope with "github.com/blah/x.Whatever", right? So I
think
> > the right approach is to instead find the final dot, and split into two
pieces
> > on that, which would be '\.\ze[^.]*$'?
> 
> I too would be curious to see a more straightforward solution. We have
> to cover all possible cases though:
> strings
> strings.Join
> crypto/md5
> crypto/md5.Size
> pkg.me.com/what/is
> pkg.me.com/what/is.There
> I used the tersest (yet idiomatic) solution I could come up with that
> covers all of them.
> 
> Note that there are ambiguous cases. Take "me.Com": does it refer to the
> documentation for the package "me.Com" or for the exported member "Com"
> of package "me"? But this is a problem we should address when we fix the
> flawed interface of :Godoc. It has other problems too, like accepting
> multiple arguments even though it can't handle them. Can we do this in a
> separate issue?

Go package names don't have dots. Import paths are very strongly discouraged
from having dots in their final path component. I think we can ignore the
existence of a "me.Com" package, which means splitting on the final dot is the
most accurate solution, and handles all your test cases.

[kisielk, 2013-06-27 06:52:52]

On 2013/06/27 01:33:19, dsymonds wrote:
> https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim
> File misc/vim/plugin/godoc.vim (right):
> 
>
https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
> misc/vim/plugin/godoc.vim:103: let words = split(word, '\.\ze[^/]*$')
> On 2013/06/26 19:44:58, glts wrote:
> > On 2013/06/26 05:45:49, dsymonds wrote:
> > > This seems to implement the right thing, but in a roundabout way. I
believe
> > what
> > > we want is to split words like "http.Request" into ["http", "Request"],
and
> > this
> > > change is trying to cope with "github.com/blah/x.Whatever", right? So I
> think
> > > the right approach is to instead find the final dot, and split into two
> pieces
> > > on that, which would be '\.\ze[^.]*$'?
> > 
> > I too would be curious to see a more straightforward solution. We have
> > to cover all possible cases though:
> > strings
> > strings.Join
> > crypto/md5
> > crypto/md5.Size
> > pkg.me.com/what/is
> > pkg.me.com/what/is.There
> > I used the tersest (yet idiomatic) solution I could come up with that
> > covers all of them.
> > 
> > Note that there are ambiguous cases. Take "me.Com": does it refer to the
> > documentation for the package "me.Com" or for the exported member "Com"
> > of package "me"? But this is a problem we should address when we fix the
> > flawed interface of :Godoc. It has other problems too, like accepting
> > multiple arguments even though it can't handle them. Can we do this in a
> > separate issue?
> 
> Go package names don't have dots. Import paths are very strongly discouraged
> from having dots in their final path component. I think we can ignore the
> existence of a "me.Com" package, which means splitting on the final dot is the
> most accurate solution, and handles all your test cases.

That's not entirely true, for example if the package is at the root of a
repository that uses the .vcs syntax: 
http://golang.org/cmd/go/#hdr-Remote_import_path_syntax

Of course it's probably an extreme minority of cases, but just putting it out
there that it's not always discouraged.

[dsymonds, 2013-06-27 07:00:16]

On 27 June 2013 16:52,  <kamil.kisiel@gmail.com> wrote:

> That's not entirely true, for example if the package is at the root of a
> repository that uses the .vcs syntax:
> http://golang.org/cmd/go/#hdr-Remote_import_path_syntax
>
> Of course it's probably an extreme minority of cases, but just putting
> it out there that it's not always discouraged.

It's still discouraged to do that, since it yields an import path
whose basename is different from the package's name. And I don't care
about supporting people who do that in misc/vim.

[kortschak, 2013-06-27 07:19:34]

On Thu, 2013-06-27 at 17:00 +1000, David Symonds wrote:
> It's still discouraged to do that, since it yields an import path
> whose basename is different from the package's name. And I don't care
> about supporting people who do that in misc/vim.

This breaks :Godoc for single package sub-repositories on googlecode
unless people tuck their package an an additional directory as far as I
can see.

[dsymonds, 2013-06-27 07:27:40]

On 27 June 2013 17:19, Dan Kortschak <dan.kortschak@adelaide.edu.au> wrote:

> On Thu, 2013-06-27 at 17:00 +1000, David Symonds wrote:
>> It's still discouraged to do that, since it yields an import path
>> whose basename is different from the package's name. And I don't care
>> about supporting people who do that in misc/vim.
>
> This breaks :Godoc for single package sub-repositories on googlecode
> unless people tuck their package an an additional directory as far as I
> can see.

Like I said, that's a discouraged layout anyway. And you won't get
this to work when over a symbol (the names won't match).

[glts, 2013-06-27 07:56:44]

On 2013/06/27 01:33:19, dsymonds wrote:
> https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim
> File misc/vim/plugin/godoc.vim (right):
> 
>
https://codereview.appspot.com/10564043/diff/6001/misc/vim/plugin/godoc.vim#n...
> misc/vim/plugin/godoc.vim:103: let words = split(word, '\.\ze[^/]*$')
> On 2013/06/26 19:44:58, glts wrote:
> > On 2013/06/26 05:45:49, dsymonds wrote:
> > > This seems to implement the right thing, but in a roundabout way. I
believe
> > what
> > > we want is to split words like "http.Request" into ["http", "Request"],
and
> > this
> > > change is trying to cope with "github.com/blah/x.Whatever", right? So I
> think
> > > the right approach is to instead find the final dot, and split into two
> pieces
> > > on that, which would be '\.\ze[^.]*$'?
> > 
> > I too would be curious to see a more straightforward solution. We have
> > to cover all possible cases though:
> > strings
> > strings.Join
> > crypto/md5
> > crypto/md5.Size
> > pkg.me.com/what/is
> > pkg.me.com/what/is.There
> > I used the tersest (yet idiomatic) solution I could come up with that
> > covers all of them.
> > 
> > Note that there are ambiguous cases. Take "me.Com": does it refer to the
> > documentation for the package "me.Com" or for the exported member "Com"
> > of package "me"? But this is a problem we should address when we fix the
> > flawed interface of :Godoc. It has other problems too, like accepting
> > multiple arguments even though it can't handle them. Can we do this in a
> > separate issue?
> 
> Go package names don't have dots. Import paths are very strongly discouraged
> from having dots in their final path component. I think we can ignore the
> existence of a "me.Com" package, which means splitting on the final dot is the
> most accurate solution, and handles all your test cases.

Sorry, I should have made my point clearer.

Your regexp doesn't cover "pkg.me.com/what/is". That case is why this
issue was raised in the first place.

[kortschak, 2013-06-27 10:27:19]

On Thu, 2013-06-27 at 17:27 +1000, David Symonds wrote:
> Like I said, that's a discouraged layout anyway.
> 
Where is that? It's not noted as discouraged in the document Kamiel
linked - except for some comments by some people on golang-nuts I've not
seen any thing along those lines. It would be nice it that is the case
for it to be documented.

[dsymonds, 2013-06-27 10:46:02]

Aah, I understand now.

What about splitting on the last of [/.]? Something like split(word,
'\.\ze[^./]*$')?

[kortschak, 2013-06-27 11:34:15]

Say we have an import of "code.google.com/p/repo.sub". This split still
does not get us the documentation for package sub. Its behaviour is a
little strange, though understandable, for packages like
"code.google.com/p/go.net/html".

On Thu, 2013-06-27 at 20:46 +1000, David Symonds wrote:
> What about splitting on the last of [/.]? Something like split(word,
> '\.\ze[^./]*$')?

[glts, 2013-06-27 20:18:38]

On 2013/06/27 10:46:02, dsymonds wrote:
> Aah, I understand now.
> 
> What about splitting on the last of [/.]? Something like split(word,
> '\.\ze[^./]*$')?

Yeah I considered this too. Let's do it this way.

[glts, 2013-06-27 20:20:13]

Hello dsymonds@golang.org, kamil.kisiel@gmail.com, dan.kortschak@adelaide.edu.au
(cc: dsymonds@golang.org, gobot@golang.org, golang-dev@googlegroups.com),

Please take another look.

[glts, 2013-06-27 20:22:42]

For me this fixes issue 5656.

However, :Godoc may need some work in the future.
- :Godoc accepts multiple arguments but does not handle them correctly
  (":Godoc strings Join" looks up "stringsJoin");
- :Godoc partitions the argument into package and member which it gets
  wrong on some occasions: the "me.Com" cases argued in the comments;
- the interface is inconsistent with the godoc command.
These could be fixed by changing the interface to be the same as for
godoc: ":Godoc package [name]".

But this will require more serious work, debate, and it would entail
getting rid of the current weird, undocumented "fallback" interface
(":Godoc crypto", followed by ":Godoc md5" opens "crypto/md5"!).

[dsymonds, 2013-06-28 07:52:55]

Let me think about this over the weekend. This has grown organically
and is not in a good state.

[dsymonds, 2013-07-01 06:33:37]

Okay, here's what I think should happen:

- If you invoke :Godoc with no arguments, the word under the cursor (incl dots)
should be split on the first dot and then godoc should be invoked. This turns
"net.IP" into ["godoc", "net", "IP"].

- If you invoke :Godoc with any arguments, those are passed to godoc as is; no
splitting on dots or anything. This matches how godoc works. It means if you
invoke ":Godoc net.IP" it won't work, but that's okay.

Care to change this file to match those semantics?

[glts, 2013-07-01 18:43:14]

On 2013/07/01 06:33:37, dsymonds wrote:
> Okay, here's what I think should happen:
> 
> - If you invoke :Godoc with no arguments, the word under the cursor (incl
dots)
> should be split on the first dot and then godoc should be invoked. This turns
> "net.IP" into ["godoc", "net", "IP"].

I'm having some difficulty understanding this. You say :Godoc should
split on the _first_ dot -- so you think looking up documentation for an
import path under the cursor should no longer be supported?

        import (
                "code.google.com/p/go.tools/go/types"
        )

This is the use case that this CL was originally supposed to address.
How about splitting on the last dot as we discussed? Then it would work
both inline as well as on import paths, at least most of time.

        strings.Join -> godoc strings Join
        code.google.com/p/go.tools/go/types -> godoc
code.google.com/p/go.tools/go/types

But I'm experienced with Vim, with Go less so, so it might be that I am
missing something obvious here.

> - If you invoke :Godoc with any arguments, those are passed to godoc as is; no
> splitting on dots or anything. This matches how godoc works. It means if you
> invoke ":Godoc net.IP" it won't work, but that's okay.

I agree.

> Care to change this file to match those semantics?

So you would like to do it in this CL? I can do it but maybe not before
the weekend.

[dsymonds, 2013-07-02 05:21:43]

On 2 July 2013 04:43,  <676c7473@gmail.com> wrote:

> On 2013/07/01 06:33:37, dsymonds wrote:
>>
>> Okay, here's what I think should happen:
>
>
>> - If you invoke :Godoc with no arguments, the word under the cursor
>
> (incl dots)
>>
>> should be split on the first dot and then godoc should be invoked.
>
> This turns
>>
>> "net.IP" into ["godoc", "net", "IP"].
>
>
> I'm having some difficulty understanding this. You say :Godoc should
> split on the _first_ dot -- so you think looking up documentation for an
> import path under the cursor should no longer be supported?
>
>         import (
>                 "code.google.com/p/go.tools/go/types"
>         )
>
> This is the use case that this CL was originally supposed to address.
> How about splitting on the last dot as we discussed? Then it would work
> both inline as well as on import paths, at least most of time.

I think the import path case should be handled separately. We are
getting stuck in a rathole with this string splitting. How about
adding this rule to what I propose: if :Godoc is invoked with no args,
and the cursor is on a line with exactly two quotation marks, and the
cursor is between those quote marks, use the string between those
quote marks as the single argument.

[glts, 2013-07-02 17:37:22]

On 2013/07/02 05:21:43, dsymonds wrote:
> On 2 July 2013 04:43,  <mailto:676c7473@gmail.com> wrote:
> 
> > On 2013/07/01 06:33:37, dsymonds wrote:
> >>
> >> Okay, here's what I think should happen:
> >
> >
> >> - If you invoke :Godoc with no arguments, the word under the cursor
> >
> > (incl dots)
> >>
> >> should be split on the first dot and then godoc should be invoked.
> >
> > This turns
> >>
> >> "net.IP" into ["godoc", "net", "IP"].
> >
> >
> > I'm having some difficulty understanding this. You say :Godoc should
> > split on the _first_ dot -- so you think looking up documentation for an
> > import path under the cursor should no longer be supported?
> >
> >         import (
> >                 "code.google.com/p/go.tools/go/types"
> >         )
> >
> > This is the use case that this CL was originally supposed to address.
> > How about splitting on the last dot as we discussed? Then it would work
> > both inline as well as on import paths, at least most of time.
> 
> I think the import path case should be handled separately. We are
> getting stuck in a rathole with this string splitting. How about
> adding this rule to what I propose: if :Godoc is invoked with no args,
> and the cursor is on a line with exactly two quotation marks, and the
> cursor is between those quote marks, use the string between those
> quote marks as the single argument.

Ok.

Sorry, but I'm going to pull out. Maybe someone else fancies the job.

Best,
David

[mattn, 2013-09-27 02:20:35]

[snip]

How about this:

 :Godoc code.google.com/p/go.tools/go/types#Config
 :Godoc encoding/json#Unmarshal

Or separate arguments:

 :Godoc encoding/json json.Unmarshal

[kisielk, 2013-09-27 05:26:49]

If you're going to do that, make it like the command line version. The second
"json" is unnecessary:

:Godoc encoding/json Unmarshal

could be equivalent to:

godoc encoding/json Unmarshal

[ioe, 2013-09-27 08:36:05]

On 2013/09/27 02:20:35, mattn wrote:
>  :Godoc encoding/json json.Unmarshal

I like this version most, because I can just copy'n'paste the fully qualified
identifier then
or provide a simple mapping for that.

:Godoc encoding/json Unmarshal

would be useful, if you renamed import for some reasons.

Extremely awesome would be to use godef (or later oracle) to qualify any
identifier 
and jump to the correct docs. But this might be out of scope for this change
set.

[mattn, 2013-12-18 05:57:30]

Is this closable?

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

Replacing golang-dev with golang-codereviews.

[dsymonds, 2014-01-03 00:48:48]

R=close

See my previous few comments in the review thread if you want to resurrect this
CL.