Issue 81240047: code review 81240047: misc/emacs: highlight type names in function parameter list (Closed)
Descriptionmisc/emacs: highlight type names in function parameter list
Currently go-mode does not set font-lock-type-face to type
names in a function parameter list. This patch is to set it
to highlight them.
Context matters to find type names in a parameter list. For
example, X and Y are type names in "func(X, Y)" but are not
in "func(X, Y int)", so we cannot say if they are type names
until we see int or a closing parenthesis in this case. In
this patch, we first scan a parameter list to see whether or
not the list has names, and scan it again to get positions
of type names.
All "T"s (and not "x"s and "y"s) in the following code are
highlighted with this patch.
func (T)
func (x T)
func (x, y T, z T)
func ([]T, T)
func (*T, T)
func (T) T
func (T) *T
func (T) (T)
func (T) (*T)
func (T) (T, T)
func (T) (n T, err T)
func (T) (m, n T)
func fn(T)
func fn(T, T, T)
func fn(x T)
func fn(x, y T, z T)
func (T) method(T) T
func (x T) method(T) (T, T)
func (x T) method(T) (n T, err T)
func (x func(x T), y func(T, T) T) func() T
Patch Set 1 #Patch Set 2 : diff -r dee4c7a24398 https://code.google.com/p/go #Patch Set 3 : diff -r dee4c7a24398 https://code.google.com/p/go #Patch Set 4 : diff -r dee4c7a24398 https://code.google.com/p/go #Patch Set 5 : diff -r dee4c7a24398 https://code.google.com/p/go #Patch Set 6 : diff -r dee4c7a24398 https://code.google.com/p/go #
Total comments: 6
Patch Set 7 : diff -r 1afdecdd77c1 https://code.google.com/p/go #
Total comments: 12
Patch Set 8 : diff -r 1afdecdd77c1 https://code.google.com/p/go #Patch Set 9 : diff -r 1afdecdd77c1 https://code.google.com/p/go #Patch Set 10 : diff -r 1afdecdd77c1 https://code.google.com/p/go #Patch Set 11 : diff -r 1afdecdd77c1 https://code.google.com/p/go #
Total comments: 2
Patch Set 12 : diff -r 1afdecdd77c1 https://code.google.com/p/go #Patch Set 13 : diff -r 7d2e78c502ab https://code.google.com/p/go #Patch Set 14 : diff -r 7d2e78c502ab https://code.google.com/p/go #
Total comments: 16
Patch Set 15 : diff -r 2af1cf6a6559 https://code.google.com/p/go #MessagesTotal messages: 24
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Impressive work. The following comments are just covering some preliminary
concerns, I didn't have a chance to read the implementation of the actual
parameter parsing yet.
I also noticed that this doesn't work correctly for multi-line function
definitions, e.g. for
func foo(x T1,
y T2) {}
T2 will not be fontified. (In case rietveld breaks the formatting, `x T1` and `y
T2` are on two different lines).
I also noticed that in `var x T`, T will get fontified as a type, while in `var
x, y T`, it will not. Was this change supposed to affect `var` at all? I thought
it was limited to functions (it would be a nice side-effect, but only if it
works reliably.)
Another discrepancy: in some places, like slices, we already fontify types. In
particular, for `[]pkg.Type`, we fontify `Type`, but not `pkg`. With your
changes, given `func fn(pkg.Type)`, the entire `pkg.Type` will be fontified as
the type. We need to pick one way of doing it.
https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (left):
https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el#oldc...
misc/emacs/go-mode.el:134: (defconst go-type-name-regexp (concat
"\\(?:[*(]\\)*\\(?:" go-identifier-regexp "\\.\\)?\\(" go-identifier-regexp
"\\)"))
This constant is part of the public interface of go-mode, that is other people's
code might depend on it. As such, it shouldn't be removed or renamed. Maybe
provide an alias and deprecate the old name if it's not appropriate anymore.
It's also confusing that there's a new constant, "go-typename-regexp", that has
almost the same name but does something completely different.
https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):
https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el#newc...
misc/emacs/go-mode.el:140: (defconst go--font-lock-func-param-num-groups 12)
Why 12? Is 13 too slow? Should this be customizable by the user? (Probably not;
if you have more than 12 function parameters, you're insane.)
https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el#newc...
misc/emacs/go-mode.el:296: (,(concat (go--regexp-enclose-in-symbol "func")
"[[:space:]]+(" go-type-regexp ")") 1 font-lock-type-face) ;; Method receiver
without variable name
Do we still need these two rules ('Method receiver' and 'Method receiver without
variable name') or does your parsing already take care of fontifying method
receivers?
Sign in to reply to this message.
Thank you for the quick review. I'll update the code tomorrow according to your
comment.
On 2014/03/30 07:36:59, Dominik Honnef wrote:
> Impressive work. The following comments are just covering some preliminary
> concerns, I didn't have a chance to read the implementation of the actual
> parameter parsing yet.
>
> I also noticed that this doesn't work correctly for multi-line function
> definitions, e.g. for
>
> func foo(x T1,
> y T2) {}
>
> T2 will not be fontified. (In case rietveld breaks the formatting, `x T1` and
`y
> T2` are on two different lines).
I used code of the standard library for testing, and there was no multi-line
function definitions there, so I was not really aware that we should deal with
that case. Supporting multi-line defintion should not be technically difficult,
so I'll try to highlight such cases tomorrow.
> I also noticed that in `var x T`, T will get fontified as a type, while in
`var
> x, y T`, it will not. Was this change supposed to affect `var` at all? I
thought
> it was limited to functions (it would be a nice side-effect, but only if it
> works reliably.)
Yeah, I added a regexp for "var" at line 297 of the new file. That's not a side
effect of the function parameter fontification. I should have not include that
change in this patch. I'll remove it and create a separate change for "var".
> Another discrepancy: in some places, like slices, we already fontify types. In
> particular, for `[]pkg.Type`, we fontify `Type`, but not `pkg`. With your
> changes, given `func fn(pkg.Type)`, the entire `pkg.Type` will be fontified as
> the type. We need to pick one way of doing it.
Good point. I did not notice that difference. IMO highlighting package name
makes more sense, as TypeName is defined as "identifier | QualifiedIdent". My
interpretation is that a package name in a qualified identifier is a part of a
type name.
Sign in to reply to this message.
https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el#oldc... misc/emacs/go-mode.el:134: (defconst go-type-name-regexp (concat "\\(?:[*(]\\)*\\(?:" go-identifier-regexp "\\.\\)?\\(" go-identifier-regexp "\\)")) OK, I think we should just keep this constant as is. The reason why I renamed this constant is because the term "TypeName" is used in the language spec and is defined as "TypeName = identifier | QualifiedIdent". This regexp is slightly off that because it allows leading [*(]*. go-typename-regexp, the new one I defined is defined as identifier or qualified identifier. https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el#newc... misc/emacs/go-mode.el:140: (defconst go--font-lock-func-param-num-groups 12) It's 12 for no particular reason. I just thought it's reasonable. It's not slow even if we set it to, say, 20. If 12 feels a little bit short, we can increase it to, say 16. https://codereview.appspot.com/81240047/diff/50002/misc/emacs/go-mode.el#newc... misc/emacs/go-mode.el:296: (,(concat (go--regexp-enclose-in-symbol "func") "[[:space:]]+(" go-type-regexp ")") 1 font-lock-type-face) ;; Method receiver without variable name Good point. These lines can be removed.
Sign in to reply to this message.
So I updated the patch with the following changes. - It now handles multi-line function definition correctly. It needed a little bit of hack with font-lock-extend-after-change-region-function because Font Lock mode is line-oriented by default. The function is used to extend the region to be scanned to enclose whole parenthesized expression. A few regexps were updated to allow newlines as space characters. - Package name in an identifier is now highlighted as a part of a type name. - A change for "var" is reverted. - The renaming of go-type-name-regex is reverted.
Sign in to reply to this message.
Only some minor nitpicks. I've tested your changes for a while and couldn't find any problems so far. I'll admit that some of the code here is overwhelming, but the overall architecture is documented well so I'm tempted to sign off on the changes. I've added Alan to the list of reviewers, so if he wants to he can take a look at the changes as well. I'll continue testing these changes and will LGTM them tomorrow if I can't find any other problems. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#old... misc/emacs/go-mode.el:279: (,(concat "\\(" go-identifier-regexp "\\)" "{") 1 font-lock-type-face) This needs to be adjusted as well, to highlight the `pkg.` in `foo := pkg.Type{initializier data}` https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:291: (,(concat (go--regexp-enclose-in-symbol "chan") "[[:space:]]*\\(?:<-*\\)?" go-type-name-regexp) 1 font-lock-type-face) ;; channel type What's this change for? https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:510: (defun go--find-enclosing-parentheses (position) This function is only intended for function parameter lists, right? E.g. when calling it with point in a function call, it produces absurd results. If so, it should be named differently to avoid confusion. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:544: ;; If we are at the beginning of a line, include the the end of previous one 'the' too many https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:547: (when (= (line-beginning-position) (point)) Use bolp instead ``Return t if point is at the beginning of a line'' https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:550: (when (= (line-end-position) (point)) eolp
Sign in to reply to this message.
Thank you for reviewing. I'll admit that this patch is a little bit overwhelming too, but I think its complexity directly comes from the data it needs to handle. As you know, simple regexp wouldn't work because types can be nested. It's interesting that we needed to scan a parameter list twice -- it's the simplest approach I can think of for Go's parameter list. IMO go-mode is still more readable than other languages major modes, thanks to Go's simple syntax. Particularly, the fact that no symbol table is needed to parse Go code made it easy to parse a Go code snippet in Emacs. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (left): https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#old... misc/emacs/go-mode.el:279: (,(concat "\\(" go-identifier-regexp "\\)" "{") 1 font-lock-type-face) On 2014/03/31 01:14:10, Dominik Honnef wrote: > This needs to be adjusted as well, to highlight the `pkg.` in `foo := > pkg.Type{initializier data}` Done. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:291: (,(concat (go--regexp-enclose-in-symbol "chan") "[[:space:]]*\\(?:<-*\\)?" go-type-name-regexp) 1 font-lock-type-face) ;; channel type This is an unintended modification. Reverted. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:510: (defun go--find-enclosing-parentheses (position) go--font-lock-extend-after-change-region-function is using this for function parameter lists, but this function itself is a generic utility. It just finds toplevel surrounding () around a given position. Added a comment. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:544: ;; If we are at the beginning of a line, include the the end of previous On 2014/03/31 01:14:10, Dominik Honnef wrote: > one 'the' too many Done. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:547: (when (= (line-beginning-position) (point)) Aha, that's the function I was looking for. I knew such function exists but couldn't recall it. Thank you for letting me know. https://codereview.appspot.com/81240047/diff/110001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:550: (when (= (line-end-position) (point)) On 2014/03/31 01:14:10, Dominik Honnef wrote: > eolp Done.
Sign in to reply to this message.
I found that types in the following pattern weren't correctly highlighted because of a regexp to match a type in a parameter list did not incorporate a package name. func(p.T, T) Uploaded a new patch.
Sign in to reply to this message.
https://codereview.appspot.com/81240047/diff/160001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/160001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:111: (defconst go-qualified-identifier-regexp "[[:word:][:multibyte:]]+\\.[[:word:][:multibyte:]]+") It would probably make sense to reuse go-identifier-regexp here, instead of repeating the word+multibyte regexp.
Sign in to reply to this message.
https://codereview.appspot.com/81240047/diff/160001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/160001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:111: (defconst go-qualified-identifier-regexp "[[:word:][:multibyte:]]+\\.[[:word:][:multibyte:]]+") On 2014/03/31 06:54:05, Dominik Honnef wrote: > It would probably make sense to reuse go-identifier-regexp here, instead of > repeating the word+multibyte regexp. Done.
Sign in to reply to this message.
LGTM Couldn't find any other problems.
Sign in to reply to this message.
On 2014/03/31 20:58:51, Dominik Honnef wrote: > LGTM > > Couldn't find any other problems. I haven't reviewed the CL, but it does contains a fair amount of new (and subtle) code and is not just a bug fix, so I should remind you that the tree is currently frozen until Go 1.3 to avoid regressions caused by non-essential changes. (I'm not sure how strictly the freeze applies to Emacs Lisp code. If you think this can't wait till June, make your case to Rob or Russ.)
Sign in to reply to this message.
Adding Rob and Russ to get their input on Emacs Lisp code and the freeze. I personally don't want to keep this patch on hold until June, but I'll follow the policy. If it's not OK to check it in now, I'll probably send this patch to Dominik's personal git repository on github for go-mode. On Mon, Mar 31, 2014 at 2:15 PM, <adonovan@google.com> wrote: > On 2014/03/31 20:58:51, Dominik Honnef wrote: > >> LGTM >> > > Couldn't find any other problems. >> > > I haven't reviewed the CL, but it does contains a fair amount of new > (and subtle) code and is not just a bug fix, so I should remind you that > the tree is currently frozen until Go 1.3 to avoid regressions caused by > non-essential changes. (I'm not sure how strictly the freeze applies > to Emacs Lisp code. If you think this can't wait till June, make your > case to Rob or Russ.) > > https://codereview.appspot.com/81240047/ >
Sign in to reply to this message.
A code freeze is a code freeze. This one doesn't fix a critical bug and contains "new (and subtle) code" so it doesn't qualify as an exception. -rob
Sign in to reply to this message.
Got it. Will ping after the 1.3 release. On Sun, Apr 6, 2014 at 10:07 PM, Rob 'Commander' Pike <r@google.com> wrote: > A code freeze is a code freeze. This one doesn't fix a critical bug and > contains "new (and subtle) code" so it doesn't qualify as an exception. > > -rob > >
Sign in to reply to this message.
I've been using this patch for a few months now. It's been working mostly fine, but there was an issue. I made the following change since it got LGTM. - go--font-lock-extend-after-change-region-function is removed This function is to extend the region to be fontified to include lines around the point, so that function parameters spanning multiple lines are fontified correctly. But fontifying multiple lines is fundamentally hard to handle, and it actually had a few drawbacks. The most notable one was, if an unterminated literal string was in parentheses, the following lines are fontified as a literal string. And when the string is terminated, the following lines are left fontified. That was really annoying when I was editing the import list. Maybe we could do something for the issue, but it'd be really complicated. I'd rather want to not fontify a function parameter list spanning multiple lines, because such style is unusual. So I simply removed the function for that. PTAL.
Sign in to reply to this message.
LGTM but wait for Alan On 2014/06/16 04:33:14, ruiu wrote: > The most notable one was, if an unterminated literal string was in parentheses, > the following lines are fontified as a literal string. And when the string is > terminated, the following lines are left fontified. That was really annoying > when I was editing the import list. Another very noticeable issue was that adding a new function in the middle of code would temporarily break all following fontification, until the closing parenthesis would be typed. > Maybe we could do something for the issue, but it'd be really complicated. I'd > rather want to not fontify a function parameter list spanning multiple lines, > because such style is unusual. So I simply removed the function for that. It's really not that unusual in third party code, just not very usual. I think it would be nice to support it, but not at the cost of either annoying behaviour (see your issue) or vastly more complex code. So I agree with removing it for now and reconsidering it in the future. I see that you also adjusted go--match-raw-string-literal – I have no way of testing that change as I'm not on Emacs 23 – is that change actually related to this feature?
Sign in to reply to this message.
> I see that you also adjusted go--match-raw-string-literal – I have no way of > testing that change as I'm not on Emacs 23 – is that change actually related to > this feature? Sorry not to mention about this change. This change is for backquotes inside comments. Such backquotes should completely be ignored because they cannot start a raw literal string. Otherwise it could bust the stack with max-lisp-eval-depth error, depending on the buffer contents.
Sign in to reply to this message.
I feel uneasy about this change. It adds 220 lines of dense, untested logic, to solve what I as a regular user of the existing code consider to be a non-problem. I'm also concerned that the running time of this logic may be quite noticeable. My Emacs experience has been deteriorating in recent years---not just in go-mode---because there is just so much code running all the time, resulting in higher latency, spinning wheels, and occasional CPU pegging and temporary display white-out. Do you have any sense of the effect of this change on the latency of various operations in a large buffer of Go code? I've only reviewed half of the code so far. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:139: ;; Maximum number of identifiers that can be highlighted as type name "as type names" https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:525: "Return points of toplevel '(' and ')' surrounding POSITION if s/toplevel/outermost/ https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:528: If toplevel '(' exists but ')' does not, it returns next blank "the next blank" https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:529: line or end-of-buffer position instead of position of closing "the position of the closing" https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:532: If starting parenthesis is not found, it returns (POSITION "the starting" https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:536: (let ((beg) (let (beg end) https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:555: the same as point. If it reaches end of line or a closing "the end of the line" https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:572: parameter list, and sets the identifier positions as the results "and set the" (elisp docstrings use imperative mood)
Sign in to reply to this message.
On 2014/06/17 17:06:21, adonovan wrote: > I feel uneasy about this change. It adds 220 lines of dense, untested logic, to > solve what I as a regular user of the existing code consider to be a > non-problem. I'm also concerned that the running time of this logic may be > quite noticeable. My Emacs experience has been deteriorating in recent > years---not just in go-mode---because there is just so much code running all the > time, resulting in higher latency, spinning wheels, and occasional CPU pegging > and temporary display white-out. Do you have any sense of the effect of this > change on the latency of various operations in a large buffer of Go code? I feel it's actually beneficial and noticeable for regular users. Particularly it makes functions with the same type, like "func (i, j int)", more readable at a glance. It's a nice thing to do as long as it's working correctly. The lack of test is a bad practice in Emacs Lisp world. Do you want me to write unit tests for go-mode? I think i,t should be doable. As to performance, I didn't notice any degradation of speed caused of this change, but I didn't measure it yet. I wrote a function to read all *.go files in the go repository for testing. I think I can use that function to measure the performance. Let me do that. I'll update this patch according to your review later.
Sign in to reply to this message.
Regarding speed: I've been using this patch (when it was still doing multi-line, too) for two months now, on averagely sized buffers, and didn't notice any performance issues. But my rig is fairly powerful, so YMMV. I do think it is a valuable addition for people who appreciate syntax highlighting (and 200 lines of wasted effort in the eyes of Rob Pike.) (Since tests entered the equation: I have tests for the indentation code (but nothing else) in a local branch. If we do decide to finally write tests for go-mode, I would contribute those.)
Sign in to reply to this message.
https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el File misc/emacs/go-mode.el (right): https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:139: ;; Maximum number of identifiers that can be highlighted as type name On 2014/06/17 17:06:21, adonovan wrote: > "as type names" Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:525: "Return points of toplevel '(' and ')' surrounding POSITION if On 2014/06/17 17:06:21, adonovan wrote: > s/toplevel/outermost/ Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:528: If toplevel '(' exists but ')' does not, it returns next blank On 2014/06/17 17:06:21, adonovan wrote: > "the next blank" Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:529: line or end-of-buffer position instead of position of closing On 2014/06/17 17:06:21, adonovan wrote: > "the position of the closing" Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:532: If starting parenthesis is not found, it returns (POSITION On 2014/06/17 17:06:21, adonovan wrote: > "the starting" Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:536: (let ((beg) On 2014/06/17 17:06:21, adonovan wrote: > (let (beg end) Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:555: the same as point. If it reaches end of line or a closing On 2014/06/17 17:06:21, adonovan wrote: > "the end of the line" Done. https://codereview.appspot.com/81240047/diff/220001/misc/emacs/go-mode.el#new... misc/emacs/go-mode.el:572: parameter list, and sets the identifier positions as the results On 2014/06/17 17:06:21, adonovan wrote: > "and set the" (elisp docstrings use imperative mood) Done.
Sign in to reply to this message.
*** Abandoned ***
Sign in to reply to this message.
Message was sent while issue was closed.
Alan, If you have further comments on this CL, please send them here or at https://github.com/dominikh/go-mode.el/pull/54. I'm proposing to merge this to Dominik's GitHub repository and wondering if you still want to finish your code review. Thanks.
Sign in to reply to this message.
|
