Skip to content
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

Port defadvice to define-advice #176

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Port defadvice to define-advice #176

merged 2 commits into from
Sep 19, 2024

Conversation

TOTBWF
Copy link
Contributor

@TOTBWF TOTBWF commented Sep 14, 2024

As of Emacs 30.1, defadvice has been marked obsolete, and can lead to byte-compilation errors. Luckily, this is pretty easy to fix: all we need to do is replace defadvice with define-advice.

As of Emacs 30.1, `defadvice` has been marked obsolete, and can lead
to byte-compilation errors. Luckily, this is pretty easy to fix: all
we need to do is replace `defadvice` with `define-advice`.
@dajva
Copy link
Owner

dajva commented Sep 15, 2024

Thanks for the PR. Hmm, fails for emacs 26.3. Wonder if there is something different with the evaluation time of the advice body between the two macros. Perhaps (require 'info) is needed in this file with the new macro?

@TOTBWF
Copy link
Contributor Author

TOTBWF commented Sep 18, 2024

It looks like it fails for 26.1 and 27.2 as well, which is odd. I've added the require, but I'm not super confident it will work; I suspect that something changed with define-advice/advice-add somewhere around the release of emacs 28, but a cursory search in the bugtracker didn't turn up anything.

@coveralls
Copy link

Coverage Status

coverage: 83.144% (+1.3%) from 81.869%
when pulling dea7621 on TOTBWF:defadvice
into 8c7bdce on dajva:master.

@dajva dajva merged commit e9ca15d into dajva:master Sep 19, 2024
14 checks passed
@dajva
Copy link
Owner

dajva commented Sep 19, 2024

Seems all versions pass checks so merged.

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.

3 participants