Use define-advice instead of defadvice

[Warbo]

The defadvice form is obsolete since Emacs 30.1, which gives warnings during bytecompilation, etc. The alternative is define-advice, but that doesn't allow the underlying function's arguments to be altered via (interactive ...), like these crux-with-region-or-foo macros were doing. Instead, we can only control the underlying function's arguments if we actually invoke it ourselves, by using :around advice (instead of before advice). The argument-choosing logic is the same, except we also add the case where we've been given explicit arguments (e.g. if we've been called programatically, rather than interactively).

---

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)

Thanks!

[Warbo]

Oh, seems this is the same as #23 (except I used define-advice and :around, instead of separate defun and advice-add with :filter-args)

[Warbo]

Here are some tests I used:

;;; crux-tests.el --- Tests for crux -*- lexical-binding: t; -*-

(require 'crux)

;;; Test `crux-with-region-or-buffer'

(defun crux-buffer-test-function (&optional start end)
  "Return the given START and END positions."
  (interactive)
  (list start end))

(ert-deftest crux-with-region-or-buffer-non-interactive-no-args-no-region ()
  "Test `crux-with-region-or-buffer` non-interactively with no args or region."
  (crux-with-region-or-buffer crux-buffer-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (should (equal (list 1 21) (crux-buffer-test-function))))
    (advice-remove 'crux-buffer-test-function 'with-region-or-buffer)))

(ert-deftest crux-with-region-or-buffer-non-interactive-no-args-with-region ()
  "Test `crux-with-region-or-buffer` non-interactively with no args but a region."
  (crux-with-region-or-buffer crux-buffer-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (set-mark 8)
        (goto-char 14)
        (should (equal (list 8 14) (crux-buffer-test-function))))
    (advice-remove 'crux-buffer-test-function 'with-region-or-buffer)))

(ert-deftest crux-with-region-or-buffer-non-interactive-with-args ()
  "Test `crux-with-region-or-buffer` non-interactively with args."
  (crux-with-region-or-buffer crux-buffer-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (should (equal (list 2 5) (crux-buffer-test-function 2 5))))
    (advice-remove 'crux-buffer-test-function 'with-region-or-buffer)))

(ert-deftest crux-with-region-or-buffer-interactive-with-region ()
  "Test `crux-with-region-or-buffer` interactively with a region."
  (crux-with-region-or-buffer crux-buffer-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 8)
        (set-mark 14)
        (let ((last-command this-command))
          (should (equal (list 8 14) (call-interactively 'crux-buffer-test-function)))))
    (advice-remove 'crux-buffer-test-function 'with-region-or-buffer)))

(ert-deftest crux-with-region-or-buffer-interactive-no-region ()
  "Test `crux-with-region-or-buffer` interactively with no region."
  (crux-with-region-or-buffer crux-buffer-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (let ((last-command this-command))
          (should (equal (list 1 21) (call-interactively 'crux-buffer-test-function)))))
    (advice-remove 'crux-buffer-test-function 'with-region-or-buffer)))

;;; Test `crux-with-region-or-line'

(defun crux-line-test-function (&optional start end)
  "Return the given START and END positions."
  (interactive)
  (list start end))

(ert-deftest crux-with-region-or-line-non-interactive-no-args ()
  "Test `crux-with-region-or-line` non-interactively with no args."
  (crux-with-region-or-line crux-line-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 10)
        (should (equal (list 8 15) (crux-line-test-function))))
    (advice-remove 'crux-line-test-function 'with-region-or-line)))

(ert-deftest crux-with-region-or-line-non-interactive-with-args ()
  "Test `crux-with-region-or-line` non-interactively with args."
  (crux-with-region-or-line crux-line-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (should (equal (list 2 5) (crux-line-test-function 2 5))))
    (advice-remove 'crux-line-test-function 'with-region-or-line)))

(ert-deftest crux-with-region-or-line-interactive-with-region ()
  "Test `crux-with-region-or-line` interactively with a region."
  (crux-with-region-or-line crux-line-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 2)
        (set-mark 5)
        (let ((last-command this-command))
          (should (equal (list 2 5) (call-interactively 'crux-line-test-function)))))
    (advice-remove 'crux-line-test-function 'with-region-or-line)))

(ert-deftest crux-with-region-or-line-interactive-no-region ()
  "Test `crux-with-region-or-line` interactively with no region."
  (crux-with-region-or-line crux-line-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 10)
        (let ((last-command this-command))
          (should (equal (list 8 15) (call-interactively 'crux-line-test-function)))))
    (advice-remove 'crux-line-test-function 'with-region-or-line)))

;;; Test `crux-with-region-or-sexp-or-line'

(defun crux-sexp-test-function (&optional start end)
  "Return the given START and END positions."
  (interactive)
  (list start end))

(ert-deftest crux-with-region-or-sexp-or-line-non-interactive-sexp ()
  "Test `crux-with-region-or-sexp-or-line` non-interactively on a sexp."
  (crux-with-region-or-sexp-or-line crux-sexp-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "(foo bar)\nbaz")
        (goto-char 2)
        (should (equal (list 1 10) (crux-sexp-test-function))))
    (advice-remove 'crux-sexp-test-function 'with-region-or-sexp-or-line)))

(ert-deftest crux-with-region-or-sexp-or-line-non-interactive-string ()
  "Test `crux-with-region-or-sexp-or-line` non-interactively on a string."
  (crux-with-region-or-sexp-or-line crux-sexp-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "\"foo bar\"\nbaz")
        (goto-char 2)
        (should (equal (list 1 10) (crux-sexp-test-function))))
    (advice-remove 'crux-sexp-test-function 'with-region-or-sexp-or-line)))

(ert-deftest crux-with-region-or-sexp-or-line-non-interactive-line ()
  "Test `crux-with-region-or-sexp-or-line` non-interactively on a line."
  (crux-with-region-or-sexp-or-line crux-sexp-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "(foo bar)\nbaz")
        (goto-char 11)
        (should (equal (list 11 14) (crux-sexp-test-function))))
    (advice-remove 'crux-sexp-test-function 'with-region-or-sexp-or-line)))

(ert-deftest crux-with-region-or-sexp-or-line-interactive-sexp ()
  "Test `crux-with-region-or-sexp-or-line` interactively on a sexp."
  (crux-with-region-or-sexp-or-line crux-sexp-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "(foo bar)\nbaz")
        (goto-char 2)
        (let ((last-command this-command))
          (should (equal (list 1 10) (call-interactively 'crux-sexp-test-function)))))
    (advice-remove 'crux-sexp-test-function 'with-region-or-sexp-or-line)))

;;; Test `crux-with-region-or-point-to-eol'

(defun crux-eol-test-function (&optional start end)
  "Return the given START and END positions."
  (interactive)
  (list start end))

(ert-deftest crux-with-region-or-point-to-eol-non-interactive-no-args ()
  "Test `crux-with-region-or-point-to-eol` non-interactively with no args."
  (crux-with-region-or-point-to-eol crux-eol-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 10)
        (should (equal (list 10 14) (crux-eol-test-function))))
    (advice-remove 'crux-eol-test-function 'with-region-or-point-to-eol)))

(ert-deftest crux-with-region-or-point-to-eol-non-interactive-with-args ()
  "Test `crux-with-region-or-point-to-eol` non-interactively with args."
  (crux-with-region-or-point-to-eol crux-eol-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (should (equal (list 2 5) (crux-eol-test-function 2 5))))
    (advice-remove 'crux-eol-test-function 'with-region-or-point-to-eol)))

(ert-deftest crux-with-region-or-point-to-eol-interactive-with-region ()
  "Test `crux-with-region-or-point-to-eol` interactively with a region."
  (crux-with-region-or-point-to-eol crux-eol-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 2)
        (set-mark 5)
        (let ((last-command this-command))
          (should (equal (list 2 5) (call-interactively 'crux-eol-test-function)))))
    (advice-remove 'crux-eol-test-function 'with-region-or-point-to-eol)))

(ert-deftest crux-with-region-or-point-to-eol-interactive-no-region ()
  "Test `crux-with-region-or-point-to-eol` interactively with no region."
  (crux-with-region-or-point-to-eol crux-eol-test-function)
  (unwind-protect
      (with-temp-buffer
        (insert "line 1\nline 2\nline 3")
        (goto-char 10)
        (let ((last-command this-command))
          (should (equal (list 10 14) (call-interactively 'crux-eol-test-function)))))
    (advice-remove 'crux-eol-test-function 'with-region-or-point-to-eol)))