Issue 7781043: code review 7781043: misc/emacs: Add support for godef (Closed)
|
Created:
12 years, 2 months ago by Dominik Honnef Modified:
12 years, 2 months ago Reviewers:
CC:
adonovan, cw, pah, Sameer Ajmani, golang-dev Visibility:
Public. |
Descriptionmisc/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
Patch Set 1 #Patch Set 2 : diff -r dfbaf2b999a6 https://code.google.com/p/go #Patch Set 3 : diff -r dfbaf2b999a6 https://code.google.com/p/go #
Total comments: 3
MessagesTotal messages: 15
Hello adonovan@google.com, cw@f00f.org, patrick.allen.higgins@gmail.com, sameer@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/7781043/diff/5001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7781043/diff/5001/misc/emacs/go-mode.el#newcode19 misc/emacs/go-mode.el:19: ;; being used A XEmacs developer is in the process of working around this issue. We should be able to expect a fix in a future CL.
Sign in to reply to this message.
LGTM I am currently using this and really enjoying it. Thanks! https://codereview.appspot.com/7781043/diff/5001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7781043/diff/5001/misc/emacs/go-mode.el#newcod... misc/emacs/go-mode.el:868: (call-process-region (point-min) (point-max) "godef" nil outbuf nil "-i" "-t" "-f" (file-truename buffer-file-name) "-o" (number-to-string (go--position-bytes (point)))) It looks like godef can deal with an offset (-o) in the middle of an identifier, or am I missing the elisp which sets point to the beginning of the identifier?
Sign in to reply to this message.
https://codereview.appspot.com/7781043/diff/5001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7781043/diff/5001/misc/emacs/go-mode.el#newcod... misc/emacs/go-mode.el:868: (call-process-region (point-min) (point-max) "godef" nil outbuf nil "-i" "-t" "-f" (file-truename buffer-file-name) "-o" (number-to-string (go--position-bytes (point)))) On 2013/03/13 16:49:49, pah wrote: > It looks like godef can deal with an offset (-o) in the middle of an identifier, > or am I missing the elisp which sets point to the beginning of the identifier? godef deals with offsets in the middle of identifiers.
Sign in to reply to this message.
Any other comments or can we get this committed?
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. On 18 March 2013 15:55, <dominik.honnef@gmail.com> wrote: > 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/
Sign in to reply to this message.
I really like this so far. LGMT
Sign in to reply to this message.
*** 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 <adonovan@google.com>
Sign in to reply to this message.
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, <adonovan@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 <adonovan@google.com> > > > https://codereview.appspot.com/7781043/
Sign in to reply to this message.
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:adonovan@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:adonovan@google.com> > > > > > > https://codereview.appspot.com/7781043/
Sign in to reply to this message.
jOn 19 March 2013 13:27, <dominik.honnef@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.
Sign in to reply to this message.
|
