gptel icon indicating copy to clipboard operation
gptel copied to clipboard

gptel: Silence byte-compilation warnings

Open pabl0 opened this issue 9 months ago • 11 comments

Silence most low-hanging-fruit byte compilation warnings.

https://gist.github.com/pabl0/b48d221ab50914dc1cbf1196efca82d6

I think these might be real issues needing fixing?

Emacs 29.4:

gptel-rewrite.el:522:10: Warning: ‘gptel--suffix-rewrite’ is for interactive
    use only.
gptel-rewrite.el:529:10: Warning: ‘gptel--suffix-rewrite’ is for interactive
    use only.

Emacs 28.2:

gptel-rewrite.el:365:55: Warning: the function ‘rmc--add-key-description’ is
    not known to be defined.

pabl0 avatar Feb 20 '25 11:02 pabl0

Thanks. I think I fixed the gptel-rewrite warnings.

For the rest I would like to figure out why the byte-compiler thinks the docstrings of the gptel-make-* functions are too wide instead of ignoring them. Any idea?

karthink avatar Feb 22 '25 07:02 karthink

For the rest I would like to figure out why the byte-compiler thinks the docstrings of the gptel-make-* functions are too wide instead of ignoring them. Any idea?

https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-09/msg00713.html

pabl0 avatar Feb 22 '25 09:02 pabl0

Thanks. I think I fixed the gptel-rewrite warnings.

Emacs 28 is lacking the rmc--add-key-description function. Does gptel--rewrite-dispatch work correctly with it just declared but not actually implemented with older emacsen?

https://github.com/karthink/gptel/blob/5d5610dfb62d218447b210ec8c8f833c6aca262e/gptel-rewrite.el#L363-L369

pabl0 avatar Feb 22 '25 10:02 pabl0

For the rest I would like to figure out why the byte-compiler thinks the docstrings of the gptel-make-* functions are too wide instead of ignoring them. Any idea?

https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-09/msg00713.html

To clarify:

(car (last (split-string (documentation 'gptel-make-azure) "\n" t)))
==> "(fn NAME &key CURL-ARGS HOST (PROTOCOL \"https\") (HEADER (lambda nil `((\"api-key\" \\, (gptel--get-api-key))))) (KEY 'gptel-api-key) MODELS STREAM ENDPOINT REQUEST-PARAMS)"
(length (car (last (split-string (documentation 'gptel-make-azure) "\n" t))))
==> 168

describe-function wraps it nicely, but I don't think the cl-defun macro can wrap the actual docstring stored.

If I set byte-compile-docstring-max-column to 168, it compiles without warnings. Would that be a more suitable option? I don't think it makes much practical difference.

I believe this issue should go away in future versions of Emacs where the internal docstring presentation of macros will be wrapped, if I understand the emacs-devel thread correctly.

pabl0 avatar Feb 22 '25 10:02 pabl0

For the rest I would like to figure out why the byte-compiler thinks the docstrings of the gptel-make-* functions are too wide instead of ignoring them. Any idea?

https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-09/msg00713.html

Ah, thanks for the link. That explains it.

The reason I didn't merge it immediately is that I wasn't sure what (not docstrings) does:

  docstrings  docstrings that are too wide (longer than
              `byte-compile-docstring-max-column' or
              `fill-column' characters, whichever is bigger) or
              have other stylistic issues.

What does "other stylistic issues" mean? Is that something we should care about?

karthink avatar Mar 13 '25 04:03 karthink

What does "other stylistic issues" mean?  Is that something we should care about?

I'm not entirely sure; it seems that emacs-lisp/checkdoc.el performs a variety of checks, but I'm uncertain which ones are applicable in the context of byte compilation. Naturally, setting the byte-compile-docstring-max-column to a large value would be the safest approach. However, relying on specific numbers may necessitate frequent adjustments.

pabl0 avatar Mar 25 '25 12:03 pabl0

I agree with setting byte-compile-docstring-max-column -- let's set it once in .dir-locals instead of in every elisp file, to a value just above that generated by the cl-defstruct macros.

karthink avatar Apr 19 '25 02:04 karthink

I don't think setting variables in .dir-locals.el works with byte-compilation.

pabl0 avatar Apr 19 '25 15:04 pabl0

I don't think setting variables in .dir-locals.el works with byte-compilation.

I tested it out. Whether it works depends on how byte-compilation is invoked. elisp-byte-compile-buffer copies the buffer to a temporary file (different directory) so it doesn't work. elisp-byte-compile-file respects the value of byte-compile-docstring-max-column in .dir-locals.

So it depends on how each package manager handles it.

karthink avatar Apr 19 '25 20:04 karthink

Let's just add it to all the files defining cl-structures, i.e. to all the backend libraries.

karthink avatar Apr 19 '25 20:04 karthink

Also, MELPA does not ship the .git-ignore.el file at all (unlike NonGNU-ELPA).

pabl0 avatar Apr 20 '25 21:04 pabl0

Thanks for the PR @pabl0!

karthink avatar May 24 '25 07:05 karthink