Conversation
c2b2e1f to
11c246e
Compare
pierre-rouleau
left a comment
There was a problem hiding this comment.
These changes are all valid. Lexical binding improves execution speed and provides better error checking.
@dominikh What prevents merging this in?
pierre-rouleau
left a comment
There was a problem hiding this comment.
@Hi-Angel , I don't think the `quote-style' is valid in this case, as it is normally used for creating 'elisp hyperlinks' inside the viewed docstring to the elisp variable or function.
The appropriate quoting here would be \\=' to quote each single quote character.
Since this might be ugly, in some case it might be better to remove the single quotes or to use \" surrounding the text.
pierre-rouleau
left a comment
There was a problem hiding this comment.
The change is valid, however I would have replaced num buy _num. A symbol that starts with an underscore is ignore and can be used as a reminder of the meaning. That's useful for documentation and would help if one day the num value is indeed required.
Fair enough, done.
I usually view the style as an analog to inline-code in markdown, with the benefit that it may create a hyperlink if the value refers to existing function/variable. AFAIU there's no other way to represent "inline code". To resolve the question I went to see what Emacs manual says. This section describes the style behavior. It doesn't directly describe our situation, but it has this sentence:
The point of styling the symbol in such way but avoiding hyperlink creation is only the visual appearance, i.e. using the style as inline-code. So I think the change is valid. |
After re-reading the Emacs manual I would have to agree. However when I wrote `list' or `error' inside one of my docstrings, the help buffer rendering that docstring did create a link to list and error primitive emacs functions and they were not following the word 'function' inside the docstring. I tested that with Emacs 30.2. Creating such links is certainly not the intent of the docstring in these elsip functions for go. Did you check that `error' did not create a link with to the error emacs lisp native function in your environment after byte-compiling your code? If not, I wonder if there is something else controlling that. |
Fixes a bunch of warnings like:
Warning: docstring has wrong usage of unescaped single quotes (use \=' or different quoting such as `...')
Fixes many warnings like:
In toplevel form:
test/go-comment-test.el:1:1: Warning: file has no ‘lexical-binding’ directive on its first line
Fixes a:
go-mode.el:2475:60: Warning: Unused lexical variable ‘num’
Fixes a warning:
In go-play-region:
go-mode.el:2137:13: Warning: Unused lexical variable ‘content-buf’
|
Oh yeah, you're right. I see |
|
I am curious: What was the exact docstring text and quoting with the |
Well, for example upon evaluating this code: (defun foo ()
"\\='error'"
t)…and then invoking help for It may be a bug in my Emacs version though — I am using Emacs built from master on 3 June, that's 4 months ago. |
|
I have the same behaviour on Emacs 30.2 that I built from source. I also tried it in Emacs 26.3 and it does the same. I also wonder if this is the expected behaviour. I'll ask on Emacs mailing list. |
|
Is this (still) ready to merge? |
|
Yeah, review comments have been addressed. @pierre-rouleau is discussing the Important note: as PR description says, it fixes most of the warnings, but not all of them. I just fixed the low-hanging fruit back when I opened the PR. |
|
@dominikh As far as I am concerned, the changes are fine. As for the docstring markup, I will (later this week probably), go through Emacs documentation and extract what I understand, ask specific questions to the Emacs devs and if I see problems will try to identify them clearly to them. |
go-modehas many warnings, this PR fixes most of them