Replace old- with new-style advice

[wasamasa]

Issue type

• Enhancement request

Further notes

I've come across the deprecation of old-style advice in the upcoming major Emacs release. Accomodating to this will require the following:

• Bumping the minimum Emacs version from 24.1 to 24.4 (since that's when nadvice.el was introduced)
• (Optionally) Removing now obsoleted code not needed in Emacs 24.4 or newer
• Converting old- to new-style advice (seems to have been tackled by Stefan in https://github.com/emacs-evil/evil/pull/1693#issuecomment-1618728587)

[monnier]

Side note: if compatibility with Emacs-24.1 is considered important, it can be preserved (by depending on the nadvice forward-compatibility package available from GNU ELPA). That's what I have currently on the scratch/evil branch.

[monnier]

I don't personally use Evil, so I haven't really tested my patch. Has someone given it a real test? Or any feedback about the approach I'm using? Or any other reason why it's not acceptable in its current form, or even in principle?

[axelf4]

I don't personally use Evil, so I haven't really tested my patch. Has someone given it a real test? Or any feedback about the approach I'm using? Or any other reason why it's not acceptable in its current form, or even in principle?

I took a look back in August, but ran out of time and forgot to report why it could not be outright merged, sorry.

It is mostly good, but broke tests relying on advices currently activated as a side-effect of Evil-mode being loaded, as the tests do not enable evil-mode, only evil-local-mode on a per-buffer basis. There is also more than one user who, likewise, uses evil-local-mode to enable Evil, rather than the methods documented in the manual for selectively disabling Evil in some buffers after enabling evil-mode. So while your patch is closer to the "correct" way of doing things, someone still has to fix the tests.

Is the right thing to enable/disable evil-mode before/after each tests, or does that incur too much overhead? To alleviate that it would have been nice if ERT offered test fixtures to run the setup only once. (Instead the Fixtures and Test Suites section of the ERT misses the point, with patronizing Lisp superiority commentary that has not held true since the 80s.)

[monnier]

It is mostly good, but broke tests relying on advices currently activated as a side-effect of Evil-mode being loaded, as the tests do not enable evil-mode, only evil-local-mode on a per-buffer basis.

Ah... thanks. I had completely missed this distinction. Indeed my patch didn't consider this possibility, and it will require a bit more work to accommodate that use-case.

[monnier]

OK, I pushed to scratch/evil a new version of the patches:

• Improved removal of evil-kg.el (updated rules in Makefile not to use evil-pkg.el any more and to build the file when needed).
• Updated removal of defadvice to better reproduce the current behavior. The tests now give the same 1 error as with the current head revision.

This includes a FIXME about the advice added to show-paren-function: this advice is only enabled when evil-mode is enabled whereas it seems that it should be attached to evil-local-mode like most others. I preserved that behavior but I suspect it's actually a bug.

[monnier]

Ping?

[monnier]

Pretty please?

[tomdl89]

Hey @monnier sorry you haven't got a reply on this. @axelf4 are you able to take a look? If not I'll try to find some time in the next few weeks. Unfortunately it's a busy time for me and this one looks like an issue that'd take a bit of time for me to get my head into.

[monnier]

Ping? I just rebased the scratch/evil branch over at nongnu.git .