Address most Emacs 29 warnings #3617

vemv · 2024-02-01T20:17:23Z

Fixes #3606

Addresses indentation and other warnings.

Sadly we cannot immediately upgrade the CI linting, because:

Emacs 29 deprecates lax-plist-* functions
The alternative is based on an arity that isn't present in Emacs < 29.

Similarly, xref-go-back deprecates xref-pop-marker-stack, while the former only exists on Emacs 29, so there's no possible fix, AFAIK.

However, a great deal of warnings is now fixed, which should be less noisy to users, and pave the path future refinements.

Cheers - V

Sadly we cannot immediately upgrade the linting, because: * Emacs 29 deprecates lax-plist-* functions * The alternative is based on an arity that isn't present in Emacs < 28.

vemv · 2024-02-01T20:20:03Z

cider-mode.el

-                              symbol-name)))
-          (with-demoted-errors format
-            (cider--deep-vector-to-list (read indent))))
+        (with-demoted-errors "Some :indent metadata is unreadable! \nERROR: %s"


Sadly the byte compiler kept complaining about the original shape of code, despite the many variations I tried.

I think it's ultimately because this linter assumes a string literal as an argument to with-demoted-errors.

So we have to give up on the informative string.

bbatsov · 2024-02-01T21:05:36Z

cider-mode.el

-                                (push sym functions))
-                               ((and do-var (not is-function) (not is-macro))
-                                (push sym vars)))))))))
+    (cl-labels ((handle-plist (plist)


The indentation here looks very wrong to me. If cl-labels is so broken in terms of indentation now I guess it'd be better to use a private function instead.

I agree that this one looks odd, I couldn't produce any other accepted indentation.

it'd be better to use a private function instead.

👍

I'm always worried that if people edit the code on different versions of Emacs they'd get different indentations, which always results in a more painful contribution process. (e.g. I'm still using Emacs 28, as I didn't have a big incentive to move to Emacs 29)

In general, for thread-* and cl-labels I rearranged things in such a way that Emacs 29 and <29 will agree, which is why the PR's build remain green despite the indentation changes and no CI changes.

(for thread-*, it also happens to look more clojurey)

bbatsov · 2024-02-01T21:06:18Z

cider-util.el

@@ -438,7 +438,7 @@ plugin or dependency with:
 (defvar cider-version)

 (defconst cider-manual-url "https://docs.cider.mx/cider/%s"
-  "The URL to CIDER's manual.")
+  "The URL to CIDER\='s manual.")


Hmm, that's not really a symbol.

One has to use it for all 's no matter what (which yes, looks fairly ugly)

https://emacs.stackexchange.com/posts/73048/revisions

bbatsov · 2024-02-01T21:06:25Z

cider-util.el

@@ -451,7 +451,7 @@ plugin or dependency with:
    (concat (cider-version-sans-patch) "/")))

 (defun cider-manual-url ()
-  "The CIDER manual's url."
+  "The CIDER manual\='s url."


bbatsov · 2024-02-01T21:06:32Z

cider-util.el

@@ -474,7 +474,7 @@ to."
    (buffer-string)))

 (defconst cider-refcard-url "https://github.com/clojure-emacs/cider/raw/%s/refcard/cider-refcard.pdf"
-  "The URL to CIDER's refcard.")
+  "The URL to CIDER\='s refcard.")


bbatsov · 2024-02-01T21:08:57Z

Similarly, xref-go-back deprecates xref-pop-marker-stack, while the former only exists on Emacs 29, so there's no possible fix, AFAIK.

Normally in such cases I copied stuff from the newer Emacs into cider-compat.el. (not sure if it currently exists) Backporting newer code is the only reasonable solution in such cases IMO.

I noticed that many of the changes are related to thread-first/last and cl-labels? Did they change their indentation settings? Seems like a pretty weird upstream change if so.

vemv

I noticed that many of the changes are related to thread-first/last and cl-labels? Did they change their indentation settings?

As far as I could try with Eldev (i.e. no personal settings involved), yes.

Normally in such cases I copied stuff from the newer Emacs into cider-compat.el. (not sure if it currently exists) Backporting newer code is the only reasonable solution in such cases IMO.

Yes, it was removed in c60598f

I could give it a shot. However we'd redefine lax-plist-* functions to receive an extra arity. That would seem a bit hacky and perhaps we'd be better off with the warnings.

We could upgrade the CI linter to emacs 29, but running a search/replace beforehand in the CI job for such cases? So that it doesn't complain for the cases that aren't reasonably fixable.

vemv · 2024-02-01T21:10:13Z

cider-mode.el

-                                (push sym functions))
-                               ((and do-var (not is-function) (not is-macro))
-                                (push sym vars)))))))))
+    (cl-labels ((handle-plist (plist)


I agree that this one looks odd, I couldn't produce any other accepted indentation.

it'd be better to use a private function instead.

👍

vemv · 2024-02-01T21:12:01Z

cider-util.el

@@ -438,7 +438,7 @@ plugin or dependency with:
 (defvar cider-version)

 (defconst cider-manual-url "https://docs.cider.mx/cider/%s"
-  "The URL to CIDER's manual.")
+  "The URL to CIDER\='s manual.")


One has to use it for all 's no matter what (which yes, looks fairly ugly)

https://emacs.stackexchange.com/posts/73048/revisions

bbatsov · 2024-02-25T07:55:08Z

I just saw this in the Emacs 29 changelog:

Indentation of 'cl-flet' and 'cl-labels' has changed.
These forms now indent like this:

    (cl-flet ((bla (x)
                (* x x)))
      (bla 42))

This change also affects 'cl-macrolet', 'cl-flet*' and
'cl-symbol-macrolet'.

bbatsov · 2024-02-25T07:55:58Z

cider-browse-ns.el

-                                                (funcall pred var-meta))
-                                              items))))
+    (cl-labels ((keys-from-pred
+                 (pred items)


I think we should pull the params on the same line as the function name.

bbatsov · 2024-02-25T07:56:54Z

cider-eval.el

-                 (when p
-                   (goto-char p)
-                   (message "%s" (get-char-property p 'cider-note))))))
+  (cl-labels ((goto-next-note-boundary ()


Weirdly this doesn't look like the indentation in the Emacs 29 changelog.

I am using 29.2 and can confirm that the indentation is not correct here.

p4v4n · 2024-02-25T09:21:02Z

Emacs 29 deprecates lax-plist-* functions

The alternative is based on an arity that isn't present in Emacs < 29.

IIUC it's possible to just replace lax-plist-get/put with plist-get/put.
plist-get/put got an optional 3rd argument in emacs29+ but with 2 arguments they behave the same as lax-plist-*

(Edit: Not exactly the same but similar. lax-plist-get uses equal for comparison and plist-get uses eq. https://www.gnu.org/software/emacs/manual/html_node/elisp/Plist-Access.html)

Edit2: I was wrong. plist-* fns cannot be used as a direct replacement when the keys are strings.
Some alternatives solutions are using compat29.1 lib or replacing all plists to use keywords as keys.

vemv · 2024-02-25T11:04:09Z

Edit: Not exactly the same but similar. lax-plist-get uses equal for comparison and plist-get uses eq.

It may have been that prevented me from using a direct fix - I recall trying something and see the builds fail for E<29

I'll re-take this PR in March

Address most Emacs 29 warnings

3081bfa

Sadly we cannot immediately upgrade the linting, because: * Emacs 29 deprecates lax-plist-* functions * The alternative is based on an arity that isn't present in Emacs < 28.

vemv requested a review from bbatsov February 1, 2024 20:17

vemv commented Feb 1, 2024

View reviewed changes

bbatsov reviewed Feb 1, 2024

View reviewed changes

vemv commented Feb 1, 2024

View reviewed changes

bbatsov reviewed Feb 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address most Emacs 29 warnings #3617

Address most Emacs 29 warnings #3617

vemv commented Feb 1, 2024 •

edited

vemv Feb 1, 2024

bbatsov Feb 1, 2024

vemv Feb 1, 2024

bbatsov Feb 2, 2024

vemv Feb 2, 2024

bbatsov Feb 1, 2024

vemv Feb 1, 2024

bbatsov Feb 1, 2024

bbatsov Feb 1, 2024

bbatsov commented Feb 1, 2024

vemv left a comment

vemv Feb 1, 2024

vemv Feb 1, 2024

bbatsov commented Feb 25, 2024

bbatsov Feb 25, 2024

bbatsov Feb 25, 2024

p4v4n Feb 25, 2024

p4v4n commented Feb 25, 2024 •

edited

vemv commented Feb 25, 2024

Address most Emacs 29 warnings #3617

Are you sure you want to change the base?

Address most Emacs 29 warnings #3617

Conversation

vemv commented Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Feb 1, 2024

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Feb 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p4v4n commented Feb 25, 2024 • edited

vemv commented Feb 25, 2024

vemv commented Feb 1, 2024 •

edited

p4v4n commented Feb 25, 2024 •

edited