-
-
Notifications
You must be signed in to change notification settings - Fork 211
Fix some warnings #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix some warnings #434
Conversation
c2b2e1f
to
11c246e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are all valid. Lexical binding improves execution speed and provides better error checking.
@dominikh What prevents merging this in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All is OK in the change "Don't store execution result to a unused content-buf var "
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-mode
has many warnings, this PR fixes most of them