el-patch-template

[haji-ali]

This is an implementation of the ideas in #50. I put it as a pull request to discuss the implementation and how it can be improved/refined.

All the suggested examples by @raxod502 should work. Currently, the only limitation is that ... does not work as expected inside el-patch-concat. In particular, I would expect it to match a substring so that one does not have to write the full string if one wants to replace a single word in a long string. The implementation of such a functionality is not difficult but I wanted to get some feedback before complicating the code further.

Also, currently el-patch-template simply returns the patch form rather than yanking it to the kill-ring or executing it. For now, this can be done with

(kill-new (format "%S" (el-patch-template ...))
(eval (el-patch-template ...))

respectively. (In this instance ... should be filled. el-patch-template is not magic :) )

Any feedback on the syntax or my lisp is much appreciated (I am not an expert with lisp).

[haji-ali]

String substitution now works as well. Now an el-patch-template with a string can be short and sweet

(el-patch-template
 (defun (el-patch-swap restart-emacs radian-new-emacs))
 (el-patch-concat
   (el-patch-swap
     "Restart Emacs."
     "Start a new Emacs session without killing the current one.")
   ...
   (el-patch-swap "restarted" "started")
   ...
   (el-patch-swap "restarted" "started")
   ...
   (el-patch-swap "restarted" "started")
   ...)
 (el-patch-swap
   (save-buffers-kill-emacs)
   (restart-emacs--launch-other-emacs restart-args)))

[raxod502]

I made a few small comments, although I didn't look through the whole implementation. If it works, that's the most important thing. I'm happy to advise on more specific parts if there are places you think should be improved.

Side note---we should add el-patch-template.el to the for_compile and for_checkdoc lists in Makefile.

[haji-ali]

Thanks @raxod502. I've tested different forms and everything seems to work. Hopefully I didn't miss a use-case.

I've tried to follow the documentation standards as you suggested. I added a few questions in the code review that I would appreciate your input on (including a question about how to correctly handle a checkdoc failure).

Also, regarding your suggestions in #50. How about I create two versions:

• el-patch-template evaluates the el-patch definition. If in init file prints a warning about the evaluation speed (not sure how to detect this in a standard Emacs config). If in compile time expands into the el-patch-* definition (Also not sure how to do that last one and would appreciate pointers).
• el-patch-template* which simply returns the el-patch definition (or yanks the definition to the kill-ring).

I don't know if/how I can combine all of these into one version depending on context.

[raxod502]

How about I create two versions

Yes, two entry points seems right. I gave some thought to how to make the behavior of these functions the least surprising to the user, and came up with this proposal:

• We have a macro called el-patch-define-template which doesn't actually compute anything. Rather it simply puts its arguments into a shared variable, just like defining a regular el-patch does (but this variable would be separate than the one used to keep track of all existing el-patch forms).
• We have an interactive command called el-patch-insert-template which selects a template from the ones defined in the shared variable, like el-patch-ediff-patch, computes its expansion, and then inserts the expansion into the current buffer at point.
• We can also have a macro called el-patch-define-compiletime-template, which will take the same arguments as el-patch-define-template, and do the same thing (namely, add arguments to a shared variable for later retrieval). However, it will also have an additional effect, which is to check whether macroexpansion is happening during compilation or not. If it is, then it will compute the template expansion and then macroexpand to the expanded template. If not, then it will skip that step and print a warning.

Then our two use cases look like this:

• Simple, standard usage: Put an el-patch-template form into your init-file. Then call el-patch-insert-template to generate and insert the expanded patch into your init-file each time it's updated.
• Advanced usage with byte-compilation: Put an el-patch-define-compiletime-template form into your init-file, and ensure that you byte-compile your init-file before loading it.

Also not sure how to do that last one

Here ya go!

(defmacro magic-macro ()
  (message "This message gets printed during macroexpansion")
  `(progn
     (message "This message gets printed at runtime")
     ,(if (bound-and-true-p byte-compile-current-file)
          '(message "The macro was expanded during byte-compilation")
        '(message "The macro was expanded at runtime"))))

(magic-macro)

And then you can see the results as follows:

% emacs -Q --batch -l magic.el
This message gets printed during macroexpansion
This message gets printed at runtime
The macro was expanded at runtime

% emacs -Q --batch -f batch-byte-compile magic.el
This message gets printed during macroexpansion

% emacs -Q --batch -l magic.elc                  
This message gets printed at runtime
The macro was expanded during byte-compilation

[haji-ali]

OK, I implemented those ideas. I like the interface you suggested, but I find the fact that el-patch-define-compiletime-template throws an error during runtime a bit counterintuitive.

Instead, I made it so that el-patch-define-compiletime-template defines and evaluates a template when called in runtime. A warning is emitted in that case if el-patch-warn-on-eval-template is non-nil. If el-patch-define-compiletime-template is called in compile-time, the template is defined and its resolution is macro-expanded. To my mind, this is the most intuitive behavior but the function name might need rethinking.

In addition to el-patch-insert-template, I also added a function el-patch-eval-template which evaluates a template.

PS: Sorry for the notifications! I just learned about drafts in github and converted the request to a draft.

[raxod502]

Sorry for the notifications! I just learned about drafts in github and converted the request to a draft

Oh, no worries, I disable notifications for newly pushed commits in any case :)

added a function el-patch-eval-template

Makes sense!

To my mind, this is the most intuitive behavior but the function name might need rethinking

Yeah, that seems pretty reasonable. Maybe could rename to el-patch-define-and-eval-template or something.

I figured we would want to have the template definition saved

Yep yep, that makes sense.

I don't know if I also need to handle the case when el-patch is not loaded

Yeah, I think we do need to handle that. It should be fairly straightforward though, I think you just need to autoload any variables that need to be defined for the macroexpansion to execute successfully even if el-patch isn't loaded, e.g.:

https://github.com/raxod502/el-patch/blob/5e823dc9a29e3be22597d93912f06119c38030d6/el-patch.el#L137-L141

[haji-ali]

I've added validation functions for templates (stolen from el-patch) and greatly simplified the implementation to avoid exceeding max-lisp-eval-depth when the template is very long.

In my tests, having my templates in my init file did not slow down my startup time significantly (especially when I autoload them). However, I am still debugging an issue where getting the source from .el.gz files fails sometimes; it seems that jka is somehow not fully loaded when my init file is executed. This might be related to my Radian config being loaded in early-init.el (see radian-software/radian#474), but I am not sure yet. (It turns out that Radian sets file-name-handler-alist to nil which means that compressed files are no longer handled by jka, leading to the failure in finding the source when the source file is compressed).

Yeah, I think we do need to handle that. It should be fairly straightforward though, I think you just need to autoload any variables that need to be defined for the macroexpansion to execute successfully even if el-patch isn't loaded

Hmm... Maybe I am misunderstanding something here. Here's what I am thinking a use-case might be:

• User generates a compiled file use-compiled-file.elc which contains el-patch templates and/or patches.
• User puts (require 'use-compiled-file) in their init file. This loads their patches, i.e., their function definitions are evaluated. Loading this compiled module does not require el-patch or el-patch-template being loaded, nor does it load them. In fact, el-patch and el-patch-template need not be even available.
• User later loads el-patch and el-patch-template (interactively or otherwise) and then is able to execute el-patch-* functions to validate their patches and/or templates.

In el-patch-template, the way I did this is by adding the template definition to el-patch--templates, making sure to define it (without loading el-patch-template) if it wasn't bound. I guess I could autoload el-patch--templates instead and rely on the user generating the autoload file and loading it correctly.

Is this what you also have in mind?
Does el-patch already do something similar? I couldn't figure it out reading the source of el-patch because I couldn't find where the resolution of the el-patch is macroexpanded.

[raxod502]

having my templates in my init file did not slow down my startup time significantly

OK, good to know. I guess most people don't care about having literally the smallest number of milliseconds of startup time, and it's fine to support having the templates expanded during startup, as long as there's also an option to not do that.

It turns out that Radian sets file-name-handler-alist to nil

Oops, my bad :)

Here's what I am thinking a use-case might be

Your use case is the exact same one I have in mind.

In el-patch-template, the way I did this is by adding the template definition to el-patch--templates, making sure to define it (without loading el-patch-template) if it wasn't bound

That's a fine approach as well. It should have the same practical effect as what I suggested (autoloading the variable definition el-patch--templates).

rely on the user generating the autoload file and loading it correctly

By "the autoload file" do you mean the generated file which contains the autoloads of the el-patch package itself? If so, then I think it should be safe to assume that will be loaded correctly. package.el, straight.el, and other package managers should all take care of this automatically.

That said, it is true that if the user is installing and loading el-patch manually, then your solution would degrade more gracefully than mine. That would be a perfectly reasonable reason to go with your way, if you prefer it.

Note that the definition of el-patch--templates in the autoload file doesn't actually contain any user data: it's literally just (defvar el-patch--templates nil) if the variable is autoloaded. It's still up to the macroexpanded code to push values onto that list at runtime.

I couldn't find where the resolution of the el-patch is macroexpanded.

So, suppose you have this in your init-file:

(el-patch-defun company-statistics--load ()
  "Restore statistics."
  (load company-statistics-file 'noerror
        (el-patch-swap nil 'nomessage)
        'nosuffix))

Then it macroexpands to this (courtesy of https://github.com/joddie/macrostep):

(el-patch--definition
 (defun company-statistics--load nil "Restore statistics."
        (load company-statistics-file 'noerror
              (el-patch-swap nil 'nomessage)
              'nosuffix)))

And then the el-patch--definition macro expands again to:

(progn
  (puthash 'defun
           '(defun company-statistics--load nil "Restore statistics."
                   (load company-statistics-file 'noerror
                         (el-patch-swap nil 'nomessage)
                         'nosuffix))
           (or
            (gethash 'company-statistics--load el-patch--patches)
            (puthash 'company-statistics--load
                     (make-hash-table :test
                                      (function equal))
                     el-patch--patches)))
  (el-patch--stealthy-eval
   (defun company-statistics--load nil "Restore statistics."
          (load company-statistics-file 'noerror 'nomessage 'nosuffix))
   "This function was patched by `el-patch'."))

Where as you can see there is a puthash that installs the full text of the patch into the variable el-patch--patches. This would normally fail if el-patch.el has not yet been loaded, since the variable would be unbound (not even nil). However, since we autoload el-patch--patches

https://github.com/raxod502/el-patch/blob/5e823dc9a29e3be22597d93912f06119c38030d6/el-patch.el#L137-L141

then the package manager will copy the following into the autoload file:

(defvar el-patch--patches (make-hash-table :test 'equal) "\
Hash table of patches that have been defined.
The keys are symbols naming the objects that have been patched.
The values are hash tables mapping definition types (symbols
`defun', `defmacro', etc.) to patch definitions, which are lists
beginning with `defun', `defmacro', etc.

Note that the symbols are from the versions of patches that have
been resolved in favor of the modified version, when a patch
renames a symbol.")

which should be loaded before anything else happens in user configuration, even if el-patch.el is not actually loaded. Then, by time we get to evaluating the el-patch-defun form (or its macroexpanded/compiled version), the variable is defined and can be updated with the patch text.

Does that answer your question?

[haji-ali]

Does that answer your question?

Thanks for taking the time to write that answer. Yes, everything is clear now.
I didn't realize that macros keep expanding. It that case, the implementation should be complete now.

I added a section in the README and I think the branch is ready to be merged. Let me know if you think other changes are needed.

[raxod502]

Thank you so much for your great work and patience! 🌹