Skip to content

Fix some warnings#434

Merged
dominikh merged 4 commits into
dominikh:masterfrom
Hi-Angel:fix-warnings
May 10, 2026
Merged

Fix some warnings#434
dominikh merged 4 commits into
dominikh:masterfrom
Hi-Angel:fix-warnings

Conversation

@Hi-Angel

Copy link
Copy Markdown
Contributor

go-mode has many warnings, this PR fixes most of them

@pierre-rouleau pierre-rouleau left a comment

Copy link
Copy Markdown
Contributor

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?

@pierre-rouleau pierre-rouleau left a comment

Copy link
Copy Markdown
Contributor

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.

@pierre-rouleau pierre-rouleau left a comment

Copy link
Copy Markdown
Contributor

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.

@pierre-rouleau pierre-rouleau left a comment

Copy link
Copy Markdown
Contributor

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 "

@Hi-Angel

Hi-Angel commented Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

@pierre-rouleau

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 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.

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:

If a symbol has a function definition and/or a variable definition, but those are irrelevant to the use of the symbol that you are documenting, you can write the words ‘symbol’ or ‘program’ before the symbol name to prevent making any hyperlink. For example […]

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.

@pierre-rouleau

pierre-rouleau commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

@Hi-Angel

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:

If a symbol has a function definition and/or a variable definition, but those are irrelevant to the use of the symbol that you are documenting, you can write the words ‘symbol’ or ‘program’ before the symbol name to prevent making any hyperlink. For example […]

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.

@Hi-Angel

Hi-Angel commented Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

Oh yeah, you're right. I see error among my changes and it's interesting that if I quote it with \\=', it still creates a link. So I ended up just using double quotes.

@pierre-rouleau

Copy link
Copy Markdown
Contributor

I am curious: What was the exact docstring text and quoting with the \\=' around error that still created a link?

@Hi-Angel

Hi-Angel commented Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

I am curious: What was the exact docstring text and quoting with the \\=' around error that still created a link?

Well, for example upon evaluating this code:

(defun foo ()
  "\\='error'"
  t)

…and then invoking help for foo I get *Help* buffer with error being a link. Reproducible with emacs -Q.

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.

@pierre-rouleau

Copy link
Copy Markdown
Contributor

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.

@dominikh

dominikh commented Oct 8, 2025

Copy link
Copy Markdown
Owner

Is this (still) ready to merge?

@Hi-Angel

Hi-Angel commented Oct 8, 2025

Copy link
Copy Markdown
Contributor Author

Yeah, review comments have been addressed. @pierre-rouleau is discussing the error auto-link situation with upstream, but it's just one line and I worked it around by simply using double-quotes.

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.

@pierre-rouleau

Copy link
Copy Markdown
Contributor

@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.

@mgirod

mgirod commented May 7, 2026

Copy link
Copy Markdown

May I pull from this fix-warnings branch or is there a reason to wait until it gets merged?
Thanks!

@Hi-Angel

Hi-Angel commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, you can.

@dominikh asked if it's ready to merge, everyone confirmed — why was it still
not merged I'm not exactly sure.

@Hi-Angel Hi-Angel closed this May 7, 2026
@Hi-Angel Hi-Angel reopened this May 7, 2026
@mgirod

mgirod commented May 10, 2026

Copy link
Copy Markdown

The fix-warnings branch is 4 commits behind dominikh/master:

0ed3c52 2025-03-11 03:02:39 +0100 Fix duplicate test name
58b0c3d 2025-03-11 02:55:47 +0100 Add a basic mode for Go assembly
602d73e 2024-06-30 08:54:37 -0700 Update README.md to fix broken link to ELPA setup instructions
636d36e 2024-06-10 14:27:11 -0400 Add go-{browse-{freesymbols,doc,assembly},rename}

Should it get rebased?
also, I still get 4 warnings:

⛔ Warning (comp): go-mode.el:1257:15: Warning: Unused lexical variable `i'
⛔ Warning (comp): go-mode.el:1721:2: Warning: docstring has wrong usage of unescaped single quotes (use = or different quoting)
⛔ Warning (comp): go-mode.el:1998:2: Warning: docstring has wrong usage of unescaped single quotes (use = or different quoting)
⛔ Warning (comp): go-mode.el:2790:22: Warning: ‘go-guess-gopath’ is an obsolete function (as of 1.7.0); GOPATH has been deprecated in favour of Go modules.

Thanks!

@mgirod

mgirod commented May 10, 2026

Copy link
Copy Markdown

BTW, my emacs version is (from ubuntu):

GNU Emacs 29.3 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.41, cairo version 1.18.0) of 2026-02-03, modified by Debian

Hi-Angel added 4 commits May 10, 2026 17:45
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’
@Hi-Angel

Copy link
Copy Markdown
Contributor Author

Should it get rebased?

Done 😊

also, I still get 4 warnings:

Yeah, the PR description says "this fixes most of the warnings", it's expected some are still left.

I didn't go full on fixing the warnings, because I wasn't sure the project isn't abandoned. I usually do a small "test PR" in such cases to make sure there's someone to at least review the PR.

As you can see this is a good rule, because in this case "testing" failed, I don't think there's anyone in this project to review a PR.

@dominikh

Copy link
Copy Markdown
Owner

why was it still not merged I'm not exactly sure.

Because I suck at PRs. See https://github.com/dominikh/.github/blob/main/CONTRIBUTING.md#pull-requests for a window into my psyche.

@dominikh dominikh merged commit 8aaaa9d into dominikh:master May 10, 2026
1 check passed
@Hi-Angel

Hi-Angel commented May 10, 2026

Copy link
Copy Markdown
Contributor Author

@dominikh oh, I see, thanks for pointing that part of README out, I admit I never saw it, because pull/merge-requests are usually well-defined in open-source ecosystem.

Perhaps, could you consider disabling PRs for your project(s)? This will simplify the situation for both you and the contributors a lot 🤔

@dominikh

Copy link
Copy Markdown
Owner

Perhaps, could you consider disabling PRs for your project(s)? This will simplify the situation for both you and the contributors a lot 🤔

I'm in the process of doing that.

I apologize for the chore this has been.

@Hi-Angel

Copy link
Copy Markdown
Contributor Author

I apologize for the chore this has been.

Oh, no worries; actually, it is me who should be apologizing for not reading the docs 😊 Thank you very much for pointing out my mistake, and even more thank you for merging the PR despite the situation!

@phikal

phikal commented May 13, 2026

Copy link
Copy Markdown

It would be nice if a release could be cut, so that the warnings won't bother users after upgrading to Emacs 31. Especially since this package is listed in the new package-autosuggest system, and can be proposed to users opening Go files without a special major mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants