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

Modernize advice definition using advice-add #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

durantschoon
Copy link

@durantschoon durantschoon commented Dec 17, 2024

Description

Modernized the advice definition by replacing the old defadvice form with the modern define-advice syntax.

Changes

  • Replaced defadvice with define-advice for er/prepare-for-more-expansions-internal
  • Maintained same functionality using cleaner, modern syntax
  • No behavioral changes, just modernization of code

Testing Done

  • Tested expand-region integration
  • Verified "e to edit" message appears in minibuffer
  • Confirmed iedit-mode activation works as expected
  • Tested editing multiple occurrences

This change requires Emacs 24.4 or later (which is already required by the package dependencies).

Replace old defadvice form with modern advice-add syntax for
er/prepare-for-more-expansions-internal. This maintains the same
functionality while using the preferred advice system introduced in
Emacs 24.4.
Copy link

@fnussbaum fnussbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! It might be better to use a named advice instead of a lambda, which would be easier to remove in user configurations (for example to aid debugging):

(define-advice er/prepare-for-more-expansions-internal
         (:around (orig-fun &rest args) evil-iedit-state)
       (let* ((result (apply orig-fun args))
              (default-msg (car result))
              (default-bindings (cdr result)))
         (cons (concat default-msg ", e to edit")
               (add-to-list 'default-bindings
                            '("e" evil-iedit-state/iedit-mode-from-expand-region)))))

@durantschoon
Copy link
Author

Ah yes, I see that is better to name it for the reasons you explain. Changed and thanks for the tip!

Copy link

@fnussbaum fnussbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!
@syl20bnr This should be ready to merge.

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.

2 participants