|
|
Created:
11 years, 1 month ago by Dominik Honnef Modified:
11 years, 1 month ago Reviewers:
CC:
golang-dev, albert.strasheim, Sameer Ajmani, cw, arthur, proppy.gmail, adonovan, rsc, bradfitz Visibility:
Public. |
Descriptionmisc/emacs: Greatly improve go-mode for Emacs.
The original go-mode is plagued with odd behaviour, lack of
behaviour typical to modes in Emacs and bugs.
This change rewrites great parts of go-mode (basically only
keeping the gofmt and godoc functions).
Additionally it adds new features such as manipulating package
imports.
For more information please see
https://groups.google.com/group/golang-nuts/browse_frm/thread/3a9d6dae3369c0b5/1efe65e2f7afb190
Fixes issue 3618.
Fixes issue 4240.
Fixes issue 4322.
Fixes issue 4671.
Fixes issue 4726.
Patch Set 1 #Patch Set 2 : diff -r 981ed43b6f82 https://code.google.com/p/go #Patch Set 3 : diff -r 0adf91947752 https://code.google.com/p/go #Patch Set 4 : diff -r 0adf91947752 https://code.google.com/p/go #
Total comments: 4
Patch Set 5 : diff -r e93de8482d59 https://code.google.com/p/go #Patch Set 6 : diff -r e93de8482d59 https://code.google.com/p/go #Patch Set 7 : diff -r e93de8482d59 https://code.google.com/p/go #Patch Set 8 : diff -r e93de8482d59 https://code.google.com/p/go #
Total comments: 40
Patch Set 9 : diff -r e93de8482d59 https://code.google.com/p/go #
Total comments: 14
Patch Set 10 : diff -r e93de8482d59 https://code.google.com/p/go #Patch Set 11 : diff -r e93de8482d59 https://code.google.com/p/go #Patch Set 12 : diff -r ba6a123a1ea3 https://code.google.com/p/go #MessagesTotal messages: 37
Hello 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.
Hello On 2013/02/16 18:10:03, Dominik Honnef wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go Spotted two minor issues, don't know if they are supposed to work yet: M-x go-import-add prints the error "Lisp nesting exceeds `max-lisp-eval-depth'" M-x go-remove-unused-imports doesn't seem to successfully remove unused imports. Running emacs-24.1-7.fc17.x86_64 here. Cheers Albert
Sign in to reply to this message.
On 2013/02/17 18:54:51, albert.strasheim wrote: > Spotted two minor issues, don't know if they are supposed to work yet: > > M-x go-import-add prints the error "Lisp nesting exceeds `max-lisp-eval-depth'" Someone else reported the same issue. Either some kind of loop in the fs is messing it up, or people really have bigger and more nested workspaces than I was expecting. The current code uses a recursive lookup of directories. > M-x go-remove-unused-imports doesn't seem to successfully remove unused imports. This has been reported as well, but I cannot reproduce nor comprehend it. Unfortunately the other person hasn't gotten back to me yet. Could you a) run "M-: (go-unused-imports-lines)" and tell me if it returns an empty list instead of the line numbers of unused imports? And if it does, can you run "go build -o /dev/null file.go" (with the current working directory being that containing file.go) from a terminal and tell me its output? Also, is GOPATH set and exported correctly when starting Emacs?
Sign in to reply to this message.
Hello On Sun, Feb 17, 2013 at 9:02 PM, <dominik.honnef@gmail.com> wrote: > On 2013/02/17 18:54:51, albert.strasheim wrote: >> Spotted two minor issues, don't know if they are supposed to work yet: >> M-x go-import-add prints the error "Lisp nesting exceeds > `max-lisp-eval-depth'" > Someone else reported the same issue. Either some kind of loop in the fs > is messing it up, or people really have bigger and more nested > workspaces than I was expecting. The current code uses a recursive > lookup of directories. I don't think we have loops. Our GOPATH is one project with about 450 subdirectories. The deepest/longest one is: src/code.google.com/p/gocc/example/example/scanner and it has friends. >> M-x go-remove-unused-imports doesn't seem to successfully remove > > unused imports. > > This has been reported as well, but I cannot reproduce nor comprehend > it. Unfortunately the other person hasn't gotten back to me yet. Could > you a) run "M-: (go-unused-imports-lines)" and tell me if it returns an > empty list instead of the line numbers of unused imports? And if it it seems to return nil > does, can you run "go build -o /dev/null file.go" (with the current > working directory being that containing file.go) from a terminal and > tell me its output? # command-line-arguments ./foo.go:12: imported and not used: "io" > Also, is GOPATH set and exported correctly when starting Emacs? I can use $GOPATH in C-x C-f, so I think so. Regards Albert
Sign in to reply to this message.
On 2013/02/16 18:10:03, Dominik Honnef wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go This all looks great, but can we split this CL into the part that replaces existing behavior and the part(s) that adds new features? I'd like to review the latter separately. Also when I click Start Review, I get: Can't parse the patch to chunks and nothing else.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, fullung@gmail.com, sameer@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
The new patch set fixes go-import-add. I'll be working on go-remove-unused-imports with Albert Strasheim soon.
Sign in to reply to this message.
Looks great so far. One issue I had in 'dropping it in' to replace what I have is load vs loads. For now I updated things here to assume loads but the previous elisp did load. https://codereview.appspot.com/7314113/diff/14001/misc/emacs/go-mode-load.el File misc/emacs/go-mode-load.el (right): https://codereview.appspot.com/7314113/diff/14001/misc/emacs/go-mode-load.el#... misc/emacs/go-mode-load.el:1: ;;; go-mode-load.el --- Major mode for the Go programming language load or loads here? https://codereview.appspot.com/7314113/diff/14001/misc/emacs/go-mode-load.el#... misc/emacs/go-mode-load.el:74: again, load vs loads
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, fullung@gmail.com, sameer@golang.org, cw@f00f.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Changed references from -loads.el to -load.el and fixed go-remove-unused-imports
Sign in to reply to this message.
LGTM So far it's working quite well for me. Certainly better than what as there before (ie. it's not a regression). I would like to see this merged as-is and any remaining issues resolved as they are encountered.
Sign in to reply to this message.
This is great. Thank you very much for doing this. It will be great to have a mode that integrates better with standard Emacs mechanisms, and the other features are great, too. Many of my comments below are stylistic, but I did have some trouble getting some of the features to work. I may have a configuration that is non-standard in some way, but I'm not sure. I started with a fresh instance of GNU Emacs 24.2.1 with no other customizations, then installed the latest version from <https://github.com/dominikh/go-mode.el>. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:885: (defun godoc (query) I couldn't get this to work. It failed silently. Maybe there's some configuration I need to do. Consider adding more documentation about the assumptions at the top of the file. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:17: (defconst go-func-meth-regexp (concat "\\<func\\>\\s *\\(?:(\\s *" go-identifier-regexp "\\s +" go-type-regexp "\\s *)\\s *\\)?\\(" go-identifier-regexp "\\)(")) It would be nice to make `beginning-of-defun', etc. work with other top-level constructs, e.g. <type>. Also, it's okay to have long lines, but sometimes it helps to wrap them to reveal their structure through indentation. This line is an example. See `go-mode-keywords' below for a nice example. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:73: ) Avoid using closing parentheses on lines by themselves in Lisp. Instead, move it up to the end of the previous line (before the comment, of course). https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:131: (defun go--backward-irrelevant (&optional stop-at-string) It would be great to have short documentation strings for non-trivial functions like this. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:165: (setq group "^[{(")) It would be better not to use assignment here and above. Instead, put the `if' inside the `let'. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:264: proper parsing of the buffer content to allow features such as "proper" => "and proper" https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:268: Additionally to these core features, it offers various features to "Additionally" => "In addition" https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:291: recommended to look at goflymake "to look" => "that you look" https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:344: (defun gofmt () When I run this, it works, but ends with the message "Hunk applied." It would be better to say something more helpful, or nothing at all. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:531: ) Move these closing parentheses up. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:562: (defun go-download-play (url) `go-download-play' fails unless `url-request-data', `url-request-extra-headers', and `url-request-method' are set (e.g. to nil). https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:591: (defun go-import-add (arg import) `go-import-add' fails with this unhelpful error message unless GOPATH is set to a directory that contains "pkg/": (file-error "Opening directory" "no such file or directory" "/pkg/") https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:650: )) To match standard Lisp style, please move these closing parentheses up to the line with `mapconcat'. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:654: (go-root-and-paths))) 'string<)) Please break this line after the third closing parenthesis. It's confusing to have the comparator at an indentation level different than its peer argument. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:659: (setq cmd "go test -c") It would be better not to use assignment here and below. Instead, put the `if' inside the `let'. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:675: (defun go-remove-unused-imports (arg) `go-unused-imports-lines' fails silently if <go build> or <go test> fails.
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode-load.el File misc/emacs/go-mode-load.el (left): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode-load.el#... misc/emacs/go-mode-load.el:7: ;; (require 'go-mode-load) would be nice to leave the instructions
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/14001/misc/emacs/go-mode-load.el File misc/emacs/go-mode-load.el (right): https://codereview.appspot.com/7314113/diff/14001/misc/emacs/go-mode-load.el#... misc/emacs/go-mode-load.el:1: ;;; go-mode-load.el --- Major mode for the Go programming language On 2013/02/20 06:03:40, cw wrote: > load or loads here? Done. https://codereview.appspot.com/7314113/diff/14001/misc/emacs/go-mode-load.el#... misc/emacs/go-mode-load.el:74: On 2013/02/20 06:03:40, cw wrote: > again, load vs loads Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:885: (defun godoc (query) On 2013/02/21 00:29:29, arthur wrote: > I couldn't get this to work. It failed silently. Maybe there's some > configuration I need to do. Consider adding more documentation about the > assumptions at the top of the file. Pretty much the only assumption is that it can run godoc, so PATH has to be set correctly. That's about it. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:17: (defconst go-func-meth-regexp (concat "\\<func\\>\\s *\\(?:(\\s *" go-identifier-regexp "\\s +" go-type-regexp "\\s *)\\s *\\)?\\(" go-identifier-regexp "\\)(")) On 2013/02/21 00:29:29, arthur wrote: > It would be nice to make `beginning-of-defun', etc. work with other top-level > constructs, e.g. <type>. I did consider this, but first checked with other modes and they only worked on functions as well, or only sporadically on other structures. Either way, if at all, I'd rather do this in a separate CL to avoid introducing new changes in behavior that have to be reviewed. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:73: ) On 2013/02/21 00:29:29, arthur wrote: > Avoid using closing parentheses on lines by themselves in Lisp. Instead, move > it up to the end of the previous line (before the comment, of course). Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:131: (defun go--backward-irrelevant (&optional stop-at-string) On 2013/02/21 00:29:29, arthur wrote: > It would be great to have short documentation strings for non-trivial functions > like this. Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:165: (setq group "^[{(")) On 2013/02/21 00:29:29, arthur wrote: > It would be better not to use assignment here and above. Instead, put the `if' > inside the `let'. I wasn't sure having a case within an if within a function call was such a good idea, style-wise. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:264: proper parsing of the buffer content to allow features such as On 2013/02/21 00:29:29, arthur wrote: > "proper" => "and proper" Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:268: Additionally to these core features, it offers various features to On 2013/02/21 00:29:29, arthur wrote: > "Additionally" => "In addition" Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:291: recommended to look at goflymake On 2013/02/21 00:29:29, arthur wrote: > "to look" => "that you look" Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:344: (defun gofmt () On 2013/02/21 00:29:29, arthur wrote: > When I run this, it works, but ends with the message "Hunk applied." It would > be better to say something more helpful, or nothing at all. Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:531: ) On 2013/02/21 00:29:29, arthur wrote: > Move these closing parentheses up. Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:562: (defun go-download-play (url) On 2013/02/21 00:29:29, arthur wrote: > `go-download-play' fails unless `url-request-data', `url-request-extra-headers', > and `url-request-method' are set (e.g. to nil). Over here (main Emacs and Emacs with no site lisp/configuration loaded), it worked without, too. Is some other package (wrongly) setting these? I've seen other packages, including ELPA, fail before because of issues regarding the url-* variables. Nevertheless, I'm setting them to proper values now. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:591: (defun go-import-add (arg import) On 2013/02/21 00:29:29, arthur wrote: > `go-import-add' fails with this unhelpful error message unless GOPATH is set to > a directory that contains "pkg/": > > (file-error "Opening directory" "no such file or directory" "/pkg/") Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:650: )) On 2013/02/21 00:29:29, arthur wrote: > To match standard Lisp style, please move these closing parentheses up to the > line with `mapconcat'. Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:654: (go-root-and-paths))) 'string<)) On 2013/02/21 00:29:29, arthur wrote: > Please break this line after the third closing parenthesis. It's confusing to > have the comparator at an indentation level different than its peer argument. Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:659: (setq cmd "go test -c") On 2013/02/21 00:29:29, arthur wrote: > It would be better not to use assignment here and below. Instead, put the `if' > inside the `let'. Done. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:675: (defun go-remove-unused-imports (arg) On 2013/02/21 00:29:29, arthur wrote: > `go-unused-imports-lines' fails silently if <go build> or <go test> fails. Technically, go build also fails when its doing its job of telling us about unused imports. So how do we tell that apart from real failure?
Sign in to reply to this message.
Thanks for making these changes. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:885: (defun godoc (query) I've confirmed that the command is running, and that its exit value indicates success. Oh, my mistake. I'm a novice Go programmer, and I misunderstood what <godoc> was supposed to do. I was trying it on things like "make" and "map", for which it returns no results. When I give it a package name, it works. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:17: (defconst go-func-meth-regexp (concat "\\<func\\>\\s *\\(?:(\\s *" go-identifier-regexp "\\s +" go-type-regexp "\\s *)\\s *\\)?\\(" go-identifier-regexp "\\)(")) That's fine. I can always customize this by changing the regular expression. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:165: (setq group "^[{(")) In general, functional code is better than side-effecting code, especially in simple cases like this. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:562: (defun go-download-play (url) Strange. I'm running without any customizations or packages loaded other than "go-mode.el". In any case, thanks for fixing this. https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:675: (defun go-remove-unused-imports (arg) I see. That's a good question. It would be nice if <go build> offered you a way of calling it just for this purpose so that you could distinguish between the two cases. Oh, well. Maybe someone on the Go team has a suggestion.
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:885: (defun godoc (query) On 2013/02/21 18:08:03, arthur wrote: > I've confirmed that the command is running, and that its exit value indicates > success. > > Oh, my mistake. I'm a novice Go programmer, and I misunderstood what <godoc> > was supposed to do. I was trying it on things like "make" and "map", for which > it returns no results. When I give it a package name, it works. Hm, when I give it a name that doesn't exist (such as "make"), it prints a godoc error in the minibuffer; it doesn't fail silently.
Sign in to reply to this message.
Dominik, thanks for taking this on. At one point I was tempted to switch go-mode.el to use syntax-ppss but I suspected that it was Atlas's trick on Hercules and I would be left maintaining this code forever. So I'm glad to see you have stepped up. :) Your rewrite is quite complex so I'm not sure it's worth the effort to understand all the logic; my only code comments are trivial. But I have been playing with it and it seems both more accurate and noticeably faster than the previous indentation algorithm. It's also shorter. Nice work! https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:111: (define-key m "}" #'go-mode-insert-and-indent) Though #'foo is functionally equivalent to 'foo in Emacs Lisp if foo is a function, I would still recommend the traditional #' as it both serves as documentation and enables compiler optimisations (since it indicates that the name of the symbol isn't significant). https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:120: `(car (syntax-ppss))) Glad to see you're using ppss; I never understood why the old mode avoided just about the most optimised C function in Emacs. https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:134: (defun go--backward-irrelevant (&optional stop-at-string) What's the significance of two dashes in go--foo? (Private?) https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:654: (defun go-root-and-paths () I recommend adding docstrings even for short internal functions. https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:679: ;; FIXME Technically, -o /dev/null fails in quite some cases (on Prefer "TODO($USER): fix: ..." to "FIXME: ...".
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:111: (define-key m "}" #'go-mode-insert-and-indent) On 2013/02/21 18:45:49, adonovan wrote: > Though #'foo is functionally equivalent to 'foo in Emacs Lisp if foo is a > function, I would still recommend the traditional #' as it both serves as > documentation and enables compiler optimisations (since it indicates that the > name of the symbol isn't significant). I actually wasn't too sure about the meaning of #'. Last time I researched it, it was said that # causes the compiler to inline functions, which in my opinion didn't make much sense for define-key. I'd really love some documentation on #' https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:120: `(car (syntax-ppss))) On 2013/02/21 18:45:49, adonovan wrote: > Glad to see you're using ppss; I never understood why the old mode avoided just > about the most optimised C function in Emacs. Heh, two reasons, I'll start from the back: They ditched the syntax table completely to properly handle raw strings, which have no escape sequences. The syntax table can't handle that. My solution is limited to XEmacs and GNU Emacs 24.x and degrades gracefully on older versions. The other reason is: The authors of the old go-mode thought, and I quote, that emacs has no built-in way to tell you the current nesting in parentheses. Or in other words, they probably didn't know syntax-ppss :) https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:134: (defun go--backward-irrelevant (&optional stop-at-string) On 2013/02/21 18:45:49, adonovan wrote: > What's the significance of two dashes in go--foo? (Private?) Yup, private. https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:654: (defun go-root-and-paths () On 2013/02/21 18:45:49, adonovan wrote: > I recommend adding docstrings even for short internal functions. I'll sneak that in in a future CL. I really want this CL to settle down so it can be accepted without needing constant reevaluation. https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:679: ;; FIXME Technically, -o /dev/null fails in quite some cases (on On 2013/02/21 18:45:49, adonovan wrote: > Prefer "TODO($USER): fix: ..." to "FIXME: ...". Same as above, in a later CL.
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:111: (define-key m "}" #'go-mode-insert-and-indent) On 2013/02/21 19:08:34, Dominik Honnef wrote: > On 2013/02/21 18:45:49, adonovan wrote: > > Though #'foo is functionally equivalent to 'foo in Emacs Lisp if foo is a > > function, I would still recommend the traditional #' as it both serves as > > documentation and enables compiler optimisations (since it indicates that the > > name of the symbol isn't significant). > > I actually wasn't too sure about the meaning of #'. Last time I researched it, > it was said that # causes the compiler to inline functions, which in my opinion > didn't make much sense for define-key. I'd really love some documentation on #' Just as the reader expands 'x to (quote x), it expands #'x to the (function x). In most dialects of Lisp, you have to use exactly one or the other: (quote x) to get the name of the symbol, and (function x) to get the function associated with the symbol. Emacs Lisp's function application operator is slightly weird though, so it works given either the "name" or the function value. https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:120: `(car (syntax-ppss))) On 2013/02/21 19:08:34, Dominik Honnef wrote: > On 2013/02/21 18:45:49, adonovan wrote: > > Glad to see you're using ppss; I never understood why the old mode avoided > just > > about the most optimised C function in Emacs. > > Heh, two reasons, I'll start from the back: They ditched the syntax table > completely to properly handle raw strings, which have no escape sequences. The > syntax table can't handle that. My solution is limited to XEmacs and GNU Emacs > 24.x and degrades gracefully on older versions. > > The other reason is: The authors of the old go-mode thought, and I quote, that > emacs has no built-in way to tell you the current nesting in parentheses. Or in > other words, they probably didn't know syntax-ppss :) Ah. And the reviewer was afraid to mention it after they'd reinvented the wheel, I suppose. :)
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#oldco... misc/emacs/go-mode.el:111: (define-key m "}" #'go-mode-insert-and-indent) On 2013/02/21 19:14:40, adonovan wrote: > On 2013/02/21 19:08:34, Dominik Honnef wrote: > > On 2013/02/21 18:45:49, adonovan wrote: > > > Though #'foo is functionally equivalent to 'foo in Emacs Lisp if foo is a > > > function, I would still recommend the traditional #' as it both serves as > > > documentation and enables compiler optimisations (since it indicates that > the > > > name of the symbol isn't significant). > > > > I actually wasn't too sure about the meaning of #'. Last time I researched it, > > it was said that # causes the compiler to inline functions, which in my > opinion > > didn't make much sense for define-key. I'd really love some documentation on > #' > > Just as the reader expands 'x to (quote x), it expands #'x to the (function x). > In most dialects of Lisp, you have to use exactly one or the other: (quote x) to > get the name of the symbol, and (function x) to get the function associated with > the symbol. Emacs Lisp's function application operator is slightly weird > though, so it works given either the "name" or the function value. Ah. Thanks for explaining that, that cleared it up. Looking through the packages that come with GNU Emacs, I can see that there's no consensus on whether to use #' or not. I'll delay this until a future CL, for the same reason as before. https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/7314113/diff/23002/misc/emacs/go-mode.el#newco... misc/emacs/go-mode.el:120: `(car (syntax-ppss))) On 2013/02/21 19:14:40, adonovan wrote: > On 2013/02/21 19:08:34, Dominik Honnef wrote: > > On 2013/02/21 18:45:49, adonovan wrote: > > > Glad to see you're using ppss; I never understood why the old mode avoided > > just > > > about the most optimised C function in Emacs. > > > > Heh, two reasons, I'll start from the back: They ditched the syntax table > > completely to properly handle raw strings, which have no escape sequences. The > > syntax table can't handle that. My solution is limited to XEmacs and GNU Emacs > > 24.x and degrades gracefully on older versions. > > > > The other reason is: The authors of the old go-mode thought, and I quote, that > > emacs has no built-in way to tell you the current nesting in parentheses. Or > in > > other words, they probably didn't know syntax-ppss :) > > Ah. And the reviewer was afraid to mention it after they'd reinvented the > wheel, I suppose. :) Well, even the original version of the old mode implemented its own parser for some parts, so it was only made worse. There aren't that many gophers who use Emacs, and this CL already had more reviews than all other go-mode related CLs combined. Most of the previous changes just got accepted without a real review process.
Sign in to reply to this message.
https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode-load.el File misc/emacs/go-mode-load.el (left): https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode-load.el#... misc/emacs/go-mode-load.el:7: ;; (require 'go-mode-load) On 2013/02/21 10:52:16, proppy.gmail wrote: > would be nice to leave the instructions Done.
Sign in to reply to this message.
I've been using this version of go-mode for since last week and have had no problems. I'm inclined to just get this submitted. Alan's our resident emacs expert, so once he's happy, I'm willing to commit. Dominik, do you know whether this go-mode should work equally well on Gnu Emacs and X-Emacs? Alan, when you LGTM, I'll submit this. S On 2013/02/21 20:29:01, Dominik Honnef wrote: > https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode-load.el > File misc/emacs/go-mode-load.el (left): > > https://codereview.appspot.com/7314113/diff/32001/misc/emacs/go-mode-load.el#... > misc/emacs/go-mode-load.el:7: ;; (require 'go-mode-load) > On 2013/02/21 10:52:16, proppy.gmail wrote: > > would be nice to leave the instructions > > Done.
Sign in to reply to this message.
On 2013/02/25 15:36:50, Sameer Ajmani wrote: > Dominik, do you know whether this go-mode should work equally well on Gnu Emacs and X-Emacs? I have honestly no idea if it works in XEmacs. I was hoping for that to be determined during the review. The handling of raw strings should work in XEmacs, and I don't think I am using any GNU Emacs specific functions. But personally I was only able to test in GNU Emacs.
Sign in to reply to this message.
okay, I'll try this out in xemacs. I'd appreciate any feedback from xemacs users on this list. On Feb 25, 2013 10:40 AM, <dominik.honnef@gmail.com> wrote: > On 2013/02/25 15:36:50, Sameer Ajmani wrote: > >> Dominik, do you know whether this go-mode should work equally well on >> > Gnu Emacs and X-Emacs? > > I have honestly no idea if it works in XEmacs. I was hoping for that to > be determined during the review. The handling of raw strings should work > in XEmacs, and I don't think I am using any GNU Emacs specific > functions. > > But personally I was only able to test in GNU Emacs. > > https://codereview.appspot.**com/7314113/<https://codereview.appspot.com/7314... >
Sign in to reply to this message.
Syntax highlighting bug: In this code: const ( typeSingleton = 0 typeFixedSize = 1 typeVariableSize = 2 ) The "type" substrings are highlighed and shouldn't be. On 2013/02/25 17:38:58, Sameer Ajmani wrote: > okay, I'll try this out in xemacs. I'd appreciate any feedback from xemacs > users on this list. > On Feb 25, 2013 10:40 AM, <mailto:dominik.honnef@gmail.com> wrote: > > > On 2013/02/25 15:36:50, Sameer Ajmani wrote: > > > >> Dominik, do you know whether this go-mode should work equally well on > >> > > Gnu Emacs and X-Emacs? > > > > I have honestly no idea if it works in XEmacs. I was hoping for that to > > be determined during the review. The handling of raw strings should work > > in XEmacs, and I don't think I am using any GNU Emacs specific > > functions. > > > > But personally I was only able to test in GNU Emacs. > > > > > https://codereview.appspot.**com/7314113/%3Chttps://codereview.appspot.com/73...> > >
Sign in to reply to this message.
On 2013/02/25 22:17:55, Sameer Ajmani wrote: > Syntax highlighting bug: > In this code: > const ( > typeSingleton = 0 > typeFixedSize = 1 > typeVariableSize = 2 > ) > The "type" substrings are highlighed and shouldn't be. Seeing the same issue with the substring "default" in var defaultBufferSize = 1 << 20 > On 2013/02/25 17:38:58, Sameer Ajmani wrote: > > okay, I'll try this out in xemacs. I'd appreciate any feedback from xemacs > > users on this list. > > On Feb 25, 2013 10:40 AM, <mailto:dominik.honnef@gmail.com> wrote: > > > > > On 2013/02/25 15:36:50, Sameer Ajmani wrote: > > > > > >> Dominik, do you know whether this go-mode should work equally well on > > >> > > > Gnu Emacs and X-Emacs? > > > > > > I have honestly no idea if it works in XEmacs. I was hoping for that to > > > be determined during the review. The handling of raw strings should work > > > in XEmacs, and I don't think I am using any GNU Emacs specific > > > functions. > > > > > > But personally I was only able to test in GNU Emacs. > > > > > > > > > https://codereview.appspot.**com/7314113/%253Chttps://codereview.appspot.com/...> > > >
Sign in to reply to this message.
It will probably happen with all keywords/language builtins, even though I have no idea why. It should work the same as in GNU Emacs, where it's working fine. I'll have to take a look at it in a couple days On 2013/02/26 16:25:23, Sameer Ajmani wrote: > On 2013/02/25 22:17:55, Sameer Ajmani wrote: > > Syntax highlighting bug: > > In this code: > > const ( > > typeSingleton = 0 > > typeFixedSize = 1 > > typeVariableSize = 2 > > ) > > The "type" substrings are highlighed and shouldn't be. > > Seeing the same issue with the substring "default" in > var defaultBufferSize = 1 << 20 > > > > On 2013/02/25 17:38:58, Sameer Ajmani wrote: > > > okay, I'll try this out in xemacs. I'd appreciate any feedback from xemacs > > > users on this list. > > > On Feb 25, 2013 10:40 AM, <mailto:dominik.honnef@gmail.com> wrote: > > > > > > > On 2013/02/25 15:36:50, Sameer Ajmani wrote: > > > > > > > >> Dominik, do you know whether this go-mode should work equally well on > > > >> > > > > Gnu Emacs and X-Emacs? > > > > > > > > I have honestly no idea if it works in XEmacs. I was hoping for that to > > > > be determined during the review. The handling of raw strings should work > > > > in XEmacs, and I don't think I am using any GNU Emacs specific > > > > functions. > > > > > > > > But personally I was only able to test in GNU Emacs. > > > > > > > > > > > > > > https://codereview.appspot.**com/7314113/%25253Chttps://codereview.appspot.co...> > > > >
Sign in to reply to this message.
I propose that this has been going on long enough that we should just submit the CL already. It will not break Go 1.0 users immediately, because they are not using tip.
Sign in to reply to this message.
This is happening with GNU Emacs, not Xemacs. On Tue, Feb 26, 2013 at 11:49 AM, <dominik.honnef@gmail.com> wrote: > It will probably happen with all keywords/language builtins, even though > I have no idea why. It should work the same as in GNU Emacs, where it's > working fine. I'll have to take a look at it in a couple days > > > On 2013/02/26 16:25:23, Sameer Ajmani wrote: > >> On 2013/02/25 22:17:55, Sameer Ajmani wrote: >> > Syntax highlighting bug: >> > In this code: >> > const ( >> > typeSingleton = 0 >> > typeFixedSize = 1 >> > typeVariableSize = 2 >> > ) >> > The "type" substrings are highlighed and shouldn't be. >> > > Seeing the same issue with the substring "default" in >> var defaultBufferSize = 1 << 20 >> > > > > On 2013/02/25 17:38:58, Sameer Ajmani wrote: >> > > okay, I'll try this out in xemacs. I'd appreciate any feedback >> > from xemacs > >> > > users on this list. >> > > On Feb 25, 2013 10:40 AM, <mailto:dominik.honnef@gmail.**com<dominik.honnef@gmail.com>> >> wrote: >> > > >> > > > On 2013/02/25 15:36:50, Sameer Ajmani wrote: >> > > > >> > > >> Dominik, do you know whether this go-mode should work equally >> > well on > >> > > >> >> > > > Gnu Emacs and X-Emacs? >> > > > >> > > > I have honestly no idea if it works in XEmacs. I was hoping for >> > that to > >> > > > be determined during the review. The handling of raw strings >> > should work > >> > > > in XEmacs, and I don't think I am using any GNU Emacs specific >> > > > functions. >> > > > >> > > > But personally I was only able to test in GNU Emacs. >> > > > >> > > > >> > > >> > >> > > https://codereview.appspot.****com/7314113/%25253Chttps://cod** > ereview.appspot.com/7314113/ <http://codereview.appspot.com/7314113/>> > >> > > > >> > > > > https://codereview.appspot.**com/7314113/<https://codereview.appspot.com/7314... >
Sign in to reply to this message.
LGTM That's fine by me. On 26 February 2013 12:11, Russ Cox <rsc@golang.org> wrote: > I propose that this has been going on long enough that we should just > submit the CL already. It will not break Go 1.0 users immediately, because > they are not using tip. > > > > >
Sign in to reply to this message.
On 2013/02/26 17:18:41, Sameer Ajmani wrote: > This is happening with GNU Emacs, not Xemacs. Then I am afraid you are not testing the go-mode.el from this CL. Using the exact same patch in a vanilla GNU Emacs: http://stuff.fork-bomb.org/2013-02-26_18h-21m-52s.png The bug of keywords being highlighted in identifiers is actually an (old) bug of the previous go-mode.el, but has been fixed there as well.
Sign in to reply to this message.
On Tue, Feb 26, 2013 at 12:25 PM, <dominik.honnef@gmail.com> wrote: > Then I am afraid you are not testing the go-mode.el from this CL. > This is another reason we should submit this CL. People can get it straight from hg, and it will be easier to read minor fixes as separate CLs. Russ
Sign in to reply to this message.
Ok, I will commit this and report anything I find as new issues. On Tue, Feb 26, 2013 at 12:27 PM, Russ Cox <rsc@golang.org> wrote: > On Tue, Feb 26, 2013 at 12:25 PM, <dominik.honnef@gmail.com> wrote: > >> Then I am afraid you are not testing the go-mode.el from this CL. >> > > This is another reason we should submit this CL. People can get it > straight from hg, and it will be easier to read minor fixes as separate CLs. > > Russ > >
Sign in to reply to this message.
warning: cannot find dominik.honnef@gmail.com in CONTRIBUTORS Dominik, please complete a CLA as described at golang.org/doc/contribute.html#copyright. On 2013/02/26 17:29:23, Sameer Ajmani wrote: > Ok, I will commit this and report anything I find as new issues. > > On Tue, Feb 26, 2013 at 12:27 PM, Russ Cox <mailto:rsc@golang.org> wrote: > > > On Tue, Feb 26, 2013 at 12:25 PM, <mailto:dominik.honnef@gmail.com> wrote: > > > >> Then I am afraid you are not testing the go-mode.el from this CL. > >> > > > > This is another reason we should submit this CL. People can get it > > straight from hg, and it will be easier to read minor fixes as separate CLs. > > > > Russ > > > >
Sign in to reply to this message.
I submitted an Individual CLA, using the electronic form, at the same time I submitted this CL. I never received any confirmation from Google though. How do I proceed? On 2013/02/26 17:54:28, Sameer Ajmani wrote: > warning: cannot find mailto:dominik.honnef@gmail.com in CONTRIBUTORS > > Dominik, please complete a CLA as described at > golang.org/doc/contribute.html#copyright. > > > > On 2013/02/26 17:29:23, Sameer Ajmani wrote: > > Ok, I will commit this and report anything I find as new issues. > > > > On Tue, Feb 26, 2013 at 12:27 PM, Russ Cox <mailto:rsc@golang.org> wrote: > > > > > On Tue, Feb 26, 2013 at 12:25 PM, <mailto:dominik.honnef@gmail.com> wrote: > > > > > >> Then I am afraid you are not testing the go-mode.el from this CL. > > >> > > > > > > This is another reason we should submit this CL. People can get it > > > straight from hg, and it will be easier to read minor fixes as separate CLs. > > > > > > Russ > > > > > >
Sign in to reply to this message.
I've been trying to add you, but our tools are broken. Will keep fighting it. 2013/02/26 10:06:37 found I AGREE for dominik.honnef@gmail.com creating CL ... Email (login for uploading to codereview.appspot.com) [bradfitz@golang.org]: ** unknown exception encountered, please report by visiting ** http://mercurial.selenic.com/wiki/BugTracker ** Python 2.7.3 (default, Aug 1 2012, 05:14:39) [GCC 4.6.3] ** Mercurial Distributed SCM (version 2.0.2) ** Extensions loaded: codereview Traceback (most recent call last): File "/usr/bin/hg", line 38, in <module> mercurial.dispatch.run() File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 27, in run sys.exit(dispatch(request(sys.argv[1:]))) File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 64, in dispatch return _runcatch(req) File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 87, in _runcatch return _dispatch(req) File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 684, in _dispatch cmdpats, cmdoptions) File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 466, in runcommand ret = _runcommand(ui, options, cmd, d) File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 738, in _runcommand return checkargs() File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 692, in checkargs return cmdfunc() File "/usr/lib/python2.7/dist-packages/mercurial/dispatch.py", line 681, in <lambda> d = lambda: util.checksignature(func)(ui, *args, **cmdoptions) File "/usr/lib/python2.7/dist-packages/mercurial/util.py", line 458, in check return func(*args, **kwargs) File "/home/bradfitz/go/lib/codereview/codereview.py", line 1348, in change d.Flush(ui, repo) File "/home/bradfitz/go/lib/codereview/codereview.py", line 282, in Flush self.Upload(ui, repo, gofmt_just_warn=True, creating=True) File "/home/bradfitz/go/lib/codereview/codereview.py", line 353, in Upload response_body = MySend("/upload", body, content_type=ctype) File "/home/bradfitz/go/lib/codereview/codereview.py", line 2409, in MySend return MySend1(request_path, payload, content_type, timeout, force_auth, **kwargs) File "/home/bradfitz/go/lib/codereview/codereview.py", line 2477, in MySend1 self._Authenticate() File "/home/bradfitz/go/lib/codereview/codereview.py", line 2965, in _Authenticate super(HttpRpcServer, self)._Authenticate() File "/home/bradfitz/go/lib/codereview/codereview.py", line 2875, in _Authenticate credentials = self.auth_function() File "/home/bradfitz/go/lib/codereview/codereview.py", line 3025, in GetUserCredentials email = GetEmail("Email (login for uploading to %s)" % options.server) File "/home/bradfitz/go/lib/codereview/codereview.py", line 2707, in GetEmail email = raw_input(prompt + ": ").strip() EOFError: EOF when reading a line 2013/02/26 10:06:38 exit status 1 On Tue, Feb 26, 2013 at 9:56 AM, <dominik.honnef@gmail.com> wrote: > I submitted an Individual CLA, using the electronic form, at the same > time I submitted this CL. I never received any confirmation from Google > though. How do I proceed? > > On 2013/02/26 17:54:28, Sameer Ajmani wrote: > >> warning: cannot find mailto:dominik.honnef@gmail.**com<dominik.honnef@gmail.com>in CONTRIBUTORS >> > > Dominik, please complete a CLA as described at >> golang.org/doc/contribute.**html#copyright<http://golang.org/doc/contribute.html#copyright> >> . >> > > > > On 2013/02/26 17:29:23, Sameer Ajmani wrote: >> > Ok, I will commit this and report anything I find as new issues. >> > >> > On Tue, Feb 26, 2013 at 12:27 PM, Russ Cox <mailto:rsc@golang.org> >> > wrote: > >> > >> > > On Tue, Feb 26, 2013 at 12:25 PM, >> > <mailto:dominik.honnef@gmail.**com <dominik.honnef@gmail.com>> wrote: > >> > > >> > >> Then I am afraid you are not testing the go-mode.el from this CL. >> > >> >> > > >> > > This is another reason we should submit this CL. People can get it >> > > straight from hg, and it will be easier to read minor fixes as >> > separate CLs. > >> > > >> > > Russ >> > > >> > > >> > > > > https://codereview.appspot.**com/7314113/<https://codereview.appspot.com/7314... > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=6f89256bf255 *** misc/emacs: Greatly improve go-mode for Emacs. The original go-mode is plagued with odd behaviour, lack of behaviour typical to modes in Emacs and bugs. This change rewrites great parts of go-mode (basically only keeping the gofmt and godoc functions). Additionally it adds new features such as manipulating package imports. For more information please see https://groups.google.com/group/golang-nuts/browse_frm/thread/3a9d6dae3369c0b... Fixes issue 3618. Fixes issue 4240. Fixes issue 4322. Fixes issue 4671. Fixes issue 4726. R=golang-dev, fullung, sameer, cw, arthur, proppy, adonovan, rsc, bradfitz CC=golang-dev https://codereview.appspot.com/7314113 Committer: Sameer Ajmani <sameer@golang.org>
Sign in to reply to this message.
|