Remove unneeded progn.

[jellelicht]

It did not actually seem to do anything (in modern Emacs versions), and it made it difficult to package el-patch in Guix. Please ignore if this is still needed; I will simply have to patch the packaged version on our end instead 👍

[jellelicht]

NB, the circleci failures seem to have been pre-existing issues that were simply hidden because of the progn wrapper; I could have a look at what’s what if you want, but won’t without a ping

[raxod502]

Hmmm... so, the goal of the progn wrapper is to avoid el-patch being loaded at startup. If you get rid of it, then any invocation of el-patch-deftype that is not byte-compiled will trigger the autoload. However, we copy several instances of el-patch-deftype into the autoloads file later in el-patch.el, and that file is not byte-compiled (or macroexpanded). So, unless I misunderstand something, getting rid of the progn wrapper would lead to el-patch always being loaded at startup whenever its autoloads are evaluated.

[raxod502]

However, I did go ahead and fix the lint errors in 93c414f :)

[raxod502]

Just out of curiosity, what's the packaging difficulty? It might help me better understand the use case, so that we could support it without the need for downstream patching.

[jellelicht]

In guix, we have a broken packaged version of el-patch; we have an Emacs-build-system that has trouble with the progn around the macro; I found the easiest way to ‘make it work’ with a recent release of el-patch was to get rid of the progn.

I understand the problem you describe, and agree with you that simply removing the progn is not a proper fix. I now modified the ;;;autoload cookies to circumvent this issue, and it seems the autoloads now work for me; can you check to see if this is a proper solution for you? Thanks!
See the Emacs manual for why I think this is the ‘correct’ solution 😄

[jellelicht]

NB, since these autoloadable things are macros, I could replace the autoload cookie by e.g.:
;;;###autoload (autoload 'el-patch-cl-defun "el-patch" "some-docstring (or no docstring)" nil t )
Which would ensure autoload machinery knows these things are macros.

Let me know what you find the best approach and I’ll adapt my patch to make it work for you.

[jellelicht]

Another (simpler) solution would be to kick the macro to its own file (e.g. el-patch-def.el), and simply let autoload do its thing: el-patch.el will then simply (require ‘el-patch-def). Either way seems like a better solution imho than counting on autoload not understanding and therefore eagerly load a progn form.

[jellelicht]

Polite ping, @raxod502. Are you okay with the changes pushed to my branch, or should I look for a different approach?

[raxod502]

I am sorry for the delay. So, looking at your last few comments, the situation is definitely improved compared to the initial version of the PR, but it still has the problem of el-patch getting loaded if you use a form such as el-patch-defun in your init-file, whereas with the current el-patch on master, this actually doesn't happen.

However, your suggestion to move the things that should be eagerly loaded into their own file is really a better solution, since it will make things a lot simpler. Let me try to implement this real quick based on your work, so you don't have to wait any longer for back and forth.

[raxod502]

There were a lot of complications in doing quite what you suggested, but I spent an hour or so trying out different approaches and in the end I was able to settle on the latest commit, which I believe solves the problem without introducing a regression or maintenance burden. I also updated the changelog accordingly. Can you let me know if this works for your use case?

Even if the answer is no, I think this work will not go to waste, since it is an improvement on the previous implementation in terms of using the autoload system more like it was designed to be used, and it should reduce the number of things that are in the autoloads for el-patch.

[jellelicht]

@raxod502 it seems to build, and it seems to work. I haven’t tried fancy compiled-init.el things yet though. Thanks for getting my issue resolved!

[raxod502]

Awesome, no problem. Glad I could help, finally :)