Re: code review 7781043: misc/emacs: Add support for godef (issue 7781043)

[dominik...@gmail.com]

Any other comments or can we get this committed?

https://codereview.appspot.com/7781043/

[adon...@google.com]

On 2013/03/18 19:20:26, Dominik Honnef wrote:
> Any other comments or can we get this committed?

LGTM

I'll submit it.

cheers
alan


https://codereview.appspot.com/7781043/

[adon...@google.com]

On 2013/03/18 19:24:07, adonovan wrote:
> On 2013/03/18 19:20:26, Dominik Honnef wrote:
> > Any other comments or can we get this committed?

> LGTM

> I'll submit it.

> cheers
> alan

A thought: consider writing some tests (in elisp) for the code in this
file, since it is certainly above the kind of complexity for which
testing is appropriate in 'production' programs, and it's impossible to
remember all the odd corner cases you tested interactively each time you
fix another bug. It needn't be an automated test (you don't want to
depend upon emacs, godef, etc, from the main repo), just something you
can run as 'emacs --batch go-mode-test.el'. I don't know what exists
out there to facilitate writing tests in elisp, but surely something.
As always with testing, the first one is the hardest but quickly pays
for itself.



https://codereview.appspot.com/7781043/

[dominik...@gmail.com]

I'll look into it. I do have some automated testing for the odd
indentation cases, but that's on my local computer only.

I'll see what I can do wrt testing the entire mode. There are some unit
testing and behaviour testing packages for emacs.

https://codereview.appspot.com/7781043/

[dominik...@gmail.com]

Please don't wait on tests for this CL though. I'll probably need a
while to get a first test suite done and I'll deliver it in a future CL.


https://codereview.appspot.com/7781043/

[Alan Donovan]

No, of course, that was just a general suggestion.

The reason I haven't submitted this yet is that every time I so much
as look at 'hg clpatch' it fails in a new way.

[c...@f00f.org]

I really like this so far.

LGMT


https://codereview.appspot.com/7781043/

[adon...@google.com]

*** Submitted as
https://code.google.com/p/go/source/detail?r=4ad21a3b23a4 ***

misc/emacs: Add support for godef

godef[1][2] is a third party tool for printing information about
expressions, especially the location of their definition. This can be
used to implement a "jump to definition" function. Unlike
cross-language solutions like ctags, godef does not require an index,
operates on the Go AST instead of symbols and works across packages,
including the standard library.

This patch implements two new public functions: godef-describe (C-c
C-d) and godef-jump (C-d C-j). godef-describe describes the expression
at point, printing its type, and godef-jump jumps to its definition.

[1]: https://code.google.com/p/rog-go/source/browse/exp/cmd/godef/
[2]: go get code.google.com/p/rog-go/exp/cmd/godef

R=adonovan, cw, patrick.allen.higgins, sameer
CC=golang-dev
https://codereview.appspot.com/7781043

Committer: Alan Donovan <adon...@google.com>


https://codereview.appspot.com/7781043/

[Alan Donovan]

One suggestion: this code seems to assume that godef is on Emacs's
$PATH. It would be nice if it could use heuristics to find it
relative to your current Go workspace if you installed it with 'go
get'.

But I like this feature a lot.

[dominik...@gmail.com]

I'm definitely not a fan of "heuristics". A variable to set the path to
godef might be considered, but I'm wondering what's wrong with assuming
that it's in your PATH. Every binary you want to use usually has to be
somewhere in your PATH, and I think most people who use Go seriously
have GOPATH/bin in their PATH.

On 2013/03/19 15:32:14, adonovan wrote:
> One suggestion: this code seems to assume that godef is on Emacs's
> $PATH. It would be nice if it could use heuristics to find it
> relative to your current Go workspace if you installed it with 'go
> get'.

> But I like this feature a lot.

> On 19 March 2013 11:29, <mailto:adon...@google.com> wrote:
> > *** Submitted as
> > https://code.google.com/p/go/source/detail?r=4ad21a3b23a4 ***
> >
> >
> > misc/emacs: Add support for godef
> >
> > godef[1][2] is a third party tool for printing information about
> > expressions, especially the location of their definition. This can
be
> > used to implement a "jump to definition" function. Unlike
> > cross-language solutions like ctags, godef does not require an
index,
> > operates on the Go AST instead of symbols and works across packages,
> > including the standard library.
> >
> > This patch implements two new public functions: godef-describe (C-c
> > C-d) and godef-jump (C-d C-j). godef-describe describes the
expression
> > at point, printing its type, and godef-jump jumps to its definition.
> >
> > [1]: https://code.google.com/p/rog-go/source/browse/exp/cmd/godef/
> > [2]: go get code.google.com/p/rog-go/exp/cmd/godef
> >
> > R=adonovan, cw, patrick.allen.higgins, sameer
> > CC=golang-dev
> > https://codereview.appspot.com/7781043
> >

> > Committer: Alan Donovan <mailto:adon...@google.com>
> >
> >
> > https://codereview.appspot.com/7781043/



https://codereview.appspot.com/7781043/

[Alan Donovan]

jOn 19 March 2013 13:27, <dominik...@gmail.com> wrote:
> I'm definitely not a fan of "heuristics". A variable to set the path to
> godef might be considered, but I'm wondering what's wrong with assuming
> that it's in your PATH. Every binary you want to use usually has to be
> somewhere in your PATH, and I think most people who use Go seriously
> have GOPATH/bin in their PATH.

Ok, fair enough. I've been learning a lot about GOROOT and GOPATH in
the last 24 hours and also that perhaps my setup is overcomplicated.
Never mind.