Provide an option to use Emacs advice system #63

haji-ali · 2023-01-24T16:14:48Z

Advising is only done when the name of the function is the same as the original one and if el-patch-use-advice contains the relevant type (like defun). In this case, a function *@el-patch-advice is defined and the original function is advised.

Things to consider:

How this plays with variants (I don't use them, so I am out of the loop).
If we want to include a field in el-patch-deftype-alist that distinguishes advisable types from the rest, to avoid having the wrong type in el-patch-use-advice.
Currently, I don't change anything in el-patch--patches to distinguish between advised vs patched functions.
There is currently no way to advice some functions but not others (related to the previous point). Perhaps this is desirable?

Once we discuss these points, I will finalize this pull request.

haji-ali · 2023-01-25T10:22:48Z

A bigger issue that I am not sure we can resolve is this example:

(require 'el-patch)

(setq el-patch-use-advice '(defun))

(defun my/test (st)
  (message (format "Hello %s" st)))

(el-patch-defun my/test (st
                         (el-patch-add st2))
  (message (format (el-patch-swap "Hello %s"
                                  "Testing %s %s")
                   st (el-patch-add st2))))

(my/test "big" "world")

This seems to work as expected. However, byte-compiling produces a warning on the expected number of arguments.
Perhaps we should also require the exact same arguments when advising? I don't see an easy way to allow more complicated argument specification.

raxod502 · 2023-01-27T03:14:03Z

I think there's no real way to resolve that issue. You'd encounter the same problem if you used an advice normally to change the number of arguments taken by a function.

There might be a workaround with some more sophisticated pointers given to the byte-compiler, but I don't know what it would be offhand, and I don't think the behavior as implemented is problematic.

How this plays with variants (I don't use them, so I am out of the loop).

I think what we want is to use the current value of the variable el-patch-variant in the name of the advice function that is defined when executing el-patch-defun. If we do that, then variants should work as expected.

If we want to include a field in el-patch-deftype-alist that distinguishes advisable types from the rest, to avoid having the wrong type in el-patch-use-advice.

That seems prudent. On the other hand, perhaps we could synthesize this information automatically by inspecting the return value of the :classify function and saying that a form can only be patched using advice if it defines a single symbol of type function?

Currently, I don't change anything in el-patch--patches to distinguish between advised vs patched functions.

Can we treat advising versus patching in the same way as variants? That is, there is another layer of sub-hashtable in el-patch--patches, and when invoking interacting commands that act on a patch, if there is both a patch and an advice for a symbol, then you can choose, but otherwise it defaults to the single one that's available?

There is currently no way to advice some functions but not others (related to the previous point). Perhaps this is desirable?

What if we make the interface a simple boolean user option to toggle the mode? You can then set it globally (to only apply to form types that are advisable), or bind it dynamically around a single patch. That offers the maximum flexibility, while solving for the simple case transparently, using the same paradigm as is already used in the variant system.

haji-ali · 2023-02-05T19:17:17Z

I think what we want is to use the current value of the variable el-patch-variant in the name of the advice function that is defined when executing el-patch-defun. If we do that, then variants should work as expected.

Hmm... if we want to allow mixing the two patching systems, then on every call el-patch--definition, we need to evaluate the original definition and then define and add the advice. Also, we need to remove the previous advice, if any.

Can we treat advising versus patching in the same way as variants? That is, there is another layer of sub-hashtable in el-patch--patches, and when invoking interacting commands that act on a patch, if there is both a patch and an advice for a symbol, then you can choose, but otherwise it defaults to the single one that's available?

Sounds reasonable. How about we use (variant . advice) where advice is either nil or t, as the key for the hashtable instead of creating a new one?

We would also need a way to figure out the last applied patch (advice or not)

That seems prudent. On the other hand, perhaps we could synthesize this information automatically by inspecting the return value of the :classify function and saying that a form can only be patched using advice if it defines a single symbol of type function?
...
What if we make the interface a simple boolean user option to toggle the mode? You can then set it globally (to only apply to form types that are advisable), or bind it dynamically around a single patch. That offers the maximum flexibility, while solving for the simple case transparently, using the same paradigm as is already used in the variant system.

Yep, sounds good. Implemented now.

haji-ali · 2023-02-07T21:31:01Z

OK, I implemented it as I suggested (using (advice . variant) as keys). I think my concerns regarding un-patching are unwarranted. I am adding a few comments on different code lines. @raxod502, Let me know what you think.

I am dogfooding this change and so far it seems to be working well.

haji-ali · 2023-02-07T21:32:19Z

el-patch.el

@@ -222,6 +226,13 @@ This function lives halfway between `copy-sequence' and
      (cons (car tree) (el-patch--copy-semitree (cdr tree)))
    tree))

+(defun el-patch--advice-name (name variant)


This can be confusing since everywhere else, variant is already (advice . variant-name) except here where it's just variant-name.

haji-ali · 2023-02-07T21:34:31Z

el-patch.el

+          (advice-remove name
+                         (el-patch--advice-name name (cdr variant)))


Note that an advice takes precedence regardless, even if a patch was applied after it. It must be manually removed or un-patched.

haji-ali · 2023-02-07T21:35:51Z

el-patch.el

+                          ;; Patches must have the same name and
+                          ;; same number of arguments


One issue is that there's no way to tell the use that this patch was not made into an advice despite having non-nil el-patch-use-advice. I thought about showing a warning, but we would need a way to silence those warnings in init-files.

I think it's okay to have it be silent at least for now. We can add a way to communicate the information later, if people report confusion.

haji-ali · 2023-02-07T21:36:47Z

el-patch.el

-                  (let ((el-patch-variant variant))
-                    (unless (el-patch-validate name type 'nomsg)
-                      (setq warning-count (1+ warning-count)))))))))
+                  (unless (el-patch-validate name type 'nomsg nil
+                                             variant)
+                    (setq warning-count (1+ warning-count))))))))


I might be mistaken, but I think this was a bug. Setting el-patch-variant does not change the behavior of el-patch-validate. Instead the variant should be passed as an argument.

Oops. Thanks for fixing that.

haji-ali · 2023-02-07T21:38:33Z

el-patch.el

+                       (puthash ',type
+                                (gethash
+                                 ',type table
+                                 (make-hash-table :test #'equal))


I had to change the test function for the variant hashtable. Nevertheless, we should check that el-patch-variant is not a cons or a string.

el-patch.el

raxod502

✨ I'm sorry this took so long. Had a lot of problems people were reporting with GNU ELPA Mirror that took some weeks to resolve. The approach looks good. There might be problems in some edge cases but they can be addressed in the future if discovered.

el-patch.el

raxod502 · 2023-02-25T18:52:39Z

el-patch.el

@@ -151,6 +151,10 @@ loaded. You can toggle the `use-package' integration later using
  "Non-nil means to validate patches when byte-compiling."
  :type 'boolean)

+(defcustom el-patch-use-advice nil
+  "Non-nil causes el-patch to use Emacs' advice system for patching."
+  :type 'list)


If this is a list then the docstring should probably mention what happens when you set it to that value (as opposed to just non-nil).

raxod502 · 2023-02-25T18:55:22Z

el-patch.el

-                  (let ((el-patch-variant variant))
-                    (unless (el-patch-validate name type 'nomsg)
-                      (setq warning-count (1+ warning-count)))))))))
+                  (unless (el-patch-validate name type 'nomsg nil
+                                             variant)
+                    (setq warning-count (1+ warning-count))))))))


Oops. Thanks for fixing that.

raxod502 · 2023-02-25T18:57:52Z

el-patch.el

+                          ;; Patches must have the same name and
+                          ;; same number of arguments


I think it's okay to have it be silent at least for now. We can add a way to communicate the information later, if people report confusion.

Co-authored-by: Radon Rosborough <radon@intuitiveexplanations.com>

…riant.

haji-ali mentioned this pull request Jan 25, 2023

Patches get silently overridden on package update / native compilation #62

Open

haji-ali force-pushed the advice-system branch 2 times, most recently from f51f6b3 to a70205f Compare January 25, 2023 10:32

haji-ali commented Feb 7, 2023

View reviewed changes

el-patch.el Outdated Show resolved Hide resolved

raxod502 approved these changes Feb 25, 2023

View reviewed changes

haji-ali added 3 commits February 26, 2023 18:17

Provide an option to use Emacs advice system

d8e1ad9

Use a boolean for advising and :classify to restrict to functions

1354440

Implemented advice's as variants

83ff6b4

haji-ali force-pushed the advice-system branch from 352045e to 83ff6b4 Compare February 26, 2023 18:18

haji-ali and others added 3 commits February 26, 2023 18:19

Correct documentation

e567f9b

Co-authored-by: Radon Rosborough <radon@intuitiveexplanations.com>

Better documentation for el-patch-advice

a713f62

Co-authored-by: Radon Rosborough <radon@intuitiveexplanations.com>

Improve documentation and issue warning on wrong type for el-patch-va…

e482d5a

…riant.

haji-ali marked this pull request as ready for review February 26, 2023 18:40

Add documentation to Readme

0e6027d

haji-ali force-pushed the advice-system branch from 567937b to 0e6027d Compare February 26, 2023 22:29

haji-ali merged commit c4a1b43 into radian-software:main Feb 26, 2023

raxod502 mentioned this pull request Apr 6, 2023

Make el-patch-get use the only variant by default #67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide an option to use Emacs advice system #63

Provide an option to use Emacs advice system #63

haji-ali commented Jan 24, 2023

haji-ali commented Jan 25, 2023

raxod502 commented Jan 27, 2023

haji-ali commented Feb 5, 2023 •

edited

Loading

haji-ali commented Feb 7, 2023 •

edited

Loading

haji-ali Feb 7, 2023

haji-ali Feb 7, 2023

haji-ali Feb 7, 2023

raxod502 Feb 25, 2023

haji-ali Feb 7, 2023 •

edited

Loading

raxod502 Feb 25, 2023

haji-ali Feb 7, 2023

raxod502 left a comment

raxod502 Feb 25, 2023

raxod502 Feb 25, 2023

raxod502 Feb 25, 2023

		(advice-remove name
		(el-patch--advice-name name (cdr variant)))

		;; Patches must have the same name and
		;; same number of arguments

Provide an option to use Emacs advice system #63

Provide an option to use Emacs advice system #63

Conversation

haji-ali commented Jan 24, 2023

haji-ali commented Jan 25, 2023

raxod502 commented Jan 27, 2023

haji-ali commented Feb 5, 2023 • edited Loading

haji-ali commented Feb 7, 2023 • edited Loading

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

haji-ali Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raxod502 left a 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

haji-ali commented Feb 5, 2023 •

edited

Loading

haji-ali commented Feb 7, 2023 •

edited

Loading

haji-ali Feb 7, 2023 •

edited

Loading