jwiegley / use-package

A use-package declaration for simplifying your .emacs
https://jwiegley.github.io/use-package
GNU General Public License v3.0
4.41k stars 260 forks source link

Proposal for post hook variables #161

Closed syl20bnr closed 9 years ago

syl20bnr commented 9 years ago

In spacemacs the configuration is organized with configuration layers stacked on top of each other. The user has a dotfile with two functions:

Packages initialization try to use use-package as much as possible to leverage the emacs lazy-loading mechanism exposed by the macro with its various keywords.

A post-init hook could allow some users to safely and consistently override the variables that need to be set before requiring a package. Indeed spacemacs comes with default values and it can be tricky to override them in some scenarios in the spacemacs user space (i.e. the package is not lazy loaded). While I believe (need someone to confirm) that eval-after-load should make a post-config hook unnecessary I would find it very convenient and symmetric if the idea of post-init hook variable is accepted.

This feature could be a game changer for spacemacs but:

jwiegley commented 9 years ago

We have :pre-init, so I don't think :post-init is a very far stretch at all! We just have to define the right semantics so that it address a useful purpose. Can you define exactly what those semantics should be, and how they compare to :pre-init?

syl20bnr commented 9 years ago

The dotfile is decoupled from the configuration files distributed in spacemacs where use-package is put into action.

Roughly the loading sequence of spacemacs is:

  1. dotspacemacs/init
  2. initialized all the relevant packages by evaluating the configurations file containing use-package
  3. dotspacemacs/config

So the user should be able to override the :init argument defined in step 2. Now you are mentioning :pre-init I guess that if step 2 is always using pre-init it would allow the user to effectively override any setting in :init, am I wrong ?

If this is the case so I would migrate the configuration to :pre-init and we are done with the initialization. Now it would be very useful to have a symmetry with a :pre-config keyword. It would make it really easy and consistent for spacemacs users to override settings at init or config time.

syl20bnr commented 9 years ago

Or maybe I stick with :init in configuration files to keep :pre-init available to the user and we add a :post-init. So the user has full flexibility.

jwiegley commented 9 years ago

That sounds like a good solution to me. So what exactly should :post-init do, relative to what :init now does? Does it strictly just run right after :init?

Silex commented 9 years ago

@jwiegley: when you say pre-init, you mean pre-load? otherwise pre-init is also undocumented.

How about you make a table of all the keywords instead of using the narrative style? That way it's easy to see what's missing and what's not.

Silex commented 9 years ago

OMG I just realised this:

Use the :init keyword to do some stuff to initialize foo. If loading was deferred, the code is run immediately; otherwise the package is required before running the code. See below for options that defer loading of the package.

It's weird to do that... It means :init changes behavior when it's not deferred! I think the semantic we want is something for before the package was required (:init) and one for after the package was required (:config). That's how I always understood it, but apparently :init can also mean after in some cases.

Maybe renaming to :before and :after would make more sense?

thomasf commented 9 years ago

pre-init is undocumented.. And I agree that the naming of all of these entry points should probably be reconsidered. If they are to be changed a good way to do it is possibly to support the old names with deprecation warnings for a long time and then removing them.

I guess it started with only :init and :config and that was fine then but now that terminology seems a bit confusing.

Silex commented 9 years ago

@npostavs, @YoungFrog: just summoning you here for your input about renaming all these "hooks" to something more meaningful (with old names supported as deprecated of course).

I also propose to stop having undefered :init happening after package being required in the name of POLA.

thomasf commented 9 years ago

I agree that If keywords really are up for a change their relationships should also be taken into consideration.

I'm not saying I want any of this to be done but it might deserve discussion before adding even more stuff.

thomasf commented 9 years ago

@syl20bnr Have you considered wrapping use-package in a spacemacs specific macro? Your use case seems a bit divergent from the way that use-package is intended to be used.

Silex commented 9 years ago

Here's a proposal draft, with keywords in their order of execution:

thomasf commented 9 years ago

Since pre- and is already a known prefix and are analogous to before we should maybe not change how that is used. There is already a pre-load which will be confusing with an addedbefore-load if they are not already the same thing..

Maybe the first thing to do is to in pseudo code lay out all the stages of execution and possible inhibitions and then see what it all means.

Here's an an example, it has errors and I'm not sure how to quickly write pseudo code for macros:

if :enabled
   do :pinned
   do :ensure
   if :idle
      prepend wrapped :idle trigger to :config
   do :pre-load
   do :load-path
   do :defines
   ...
syl20bnr commented 9 years ago

I would keep the keywords close as there are now:

if true > pre-load-path > load-path > pre-init > init > post-init > package is autoloaded > pre-config > config > post-config

The other are properties are independent from the pipeline.

What :defines do ?

pre-config may be unecessary but symmetry is easy to explain.

syl20bnr commented 9 years ago

Edits made above ^

syl20bnr commented 9 years ago

I wonder if :load-path is necessary if we put it in :init, then :pre-load-path would be included in :pre-init.

Leading to my original proposition: pre-init > init > post-init > package is autoloaded > pre-config > config > post-config

Very straightforward and it is just a matter of documenting that load-path will happen in init then everything flows without requiring to explain it.

thomasf commented 9 years ago

I was only referring to :load-path in the context of where the :load-path keyword currently is processed in the macro, not that there should be a hook for it...

syl20bnr commented 9 years ago

So what is pre-load ?

thomasf commented 9 years ago

First thing inserted into the expanded form https://github.com/jwiegley/use-package/blob/master/use-package.el#L472

syl20bnr commented 9 years ago

Ok so look to be pre-init in my proposal, so it can be removed, I don't see the benefit of :load-path it is already trivial to add a load path in init but if we keep it then here is an updated proposal:

if > pre-init > load-path > init > post-init > autoload of package > pre-config > config > post-config

thomasf commented 9 years ago

but there is lot's of more stuff..

syl20bnr commented 9 years ago

this load-path is confusing...

syl20bnr commented 9 years ago

TL;DR the pipeline should provide only keywords that influence the loading of the package.

@thomasf This lot of more stuff are inserted between pre-init and init, I don't see why it could not be.

So we have a group composed of a multitude of fine grained keywords like :load-path :defines and :init. Looks OK to me but it could be a lot simpler with just the design above and let the user handle the granularity like he prefers in the few minimal hooks we propose. The fine grained keywords are convenience that mess up the design and make it harder to understand.

I would remove all the keywords that do not influence the loading of the package. So :commands, :if and :defer must be in, the others.... not really (except the hooks of course).

thomasf commented 9 years ago

I think were just not communicating properly, its simple.

;; this
(use-package use-package
  :pre-load (message "pre-load")
  :init (message "init")
  :config (message "config")
  :load-path "path/to/package"
  :commands use-package 
  )

;; expands to :
(progn
  (message "pre-load")
  (add-to-list 'load-path "/home/a00001/.emacs.d/path/to/package")
  (eval-when-compile
    (when
        (bound-and-true-p byte-compile-current-file)
      (condition-case err
          (require 'use-package nil t)
        (error
         (message "Error requiring %s: %s" 'use-package err)
         nil))))
  (when t nil
        (unless
            (fboundp 'use-package)
          (autoload
            (function use-package)
            "use-package" nil t))
        (message "init")
        (eval-after-load 'use-package
          `(,(lambda nil
               (if t
                   (use-package-with-elapsed-timer "Configuring package use-package"
                     (message "config"))))))
        t))
syl20bnr commented 9 years ago

The others (as mentioned at the end of my previous post) could be simple additional macros to be used in the hooks.

syl20bnr commented 9 years ago

@thomasf it is not simple to me and I started to use it very early.... the additional stuff added since...... I don't need them... they are just confusion.

syl20bnr commented 9 years ago

I want to control myself the ordering of stuff, just give me the hook as it is the philosophy in the whole emacs to configure and use something.

thomasf commented 9 years ago

The composition of it all is a tad too complicated yes.. Remember the this is use-package notload-package, it's intent is to give convenience to init.el writing and also helping out with byte compilation for things under it's scope.

syl20bnr commented 9 years ago

load-package is too simplistic, it would be a version without any hooks.

thomasf commented 9 years ago

Sorry for being unclear again, I did not know there was a package called load-package.. I was just making it up as an example of something with a intent that might be more what you want.

syl20bnr commented 9 years ago

@thomasf oh there is not (is there ?), I was not very clear sorry. I meant if we remove the hooks in my proposal then you are right it could be called load-package. But with the hooks it clearly put in place the minimal needed to configure and use the package.

Silex commented 9 years ago

So, basically this is what we have now:

if :enabled
   do :pinned
   do :ensure
   if :idle
      prepend wrapped :idle trigger to :config
   do :pre-load
   do :load-path
   do :defines
   if :bind or :commands or :mode or :interpreter or...
     treat arguments and eval stuffs
     set :defer t
   if :defer false
    load package
   if :defer true
    setup eval-after-load
   do :init

later on, when package loads:
     do :config
     do :idle after timeout

Can someone complete? Even in its current form we see there is room for improvements.

thomasf commented 9 years ago

@syl20bnr I guess that since you are doing your own emacs distribution (of sorts) you might want to implement your own macros/functions to to handle that. I have 513 instances of (use-package... in my init.el and that works without any real problems.

syl20bnr commented 9 years ago

We diverged too much, my initial proposal was not redesigning use-package :-) but extend it. At the end of the day I just need more hooks to let users overwrite stuff easily.

syl20bnr commented 9 years ago

It makes me think that initialy I did not think about new keywords, I had in mind true hook variables usable with regular add-hook so it was more about finding a good naming scheme for them.

thomasf commented 9 years ago

@syl20bnr Hooks prohibits the :disabled feature of use-package to remove all code when compiling to silence the byte compiler about undefined symbols.

I believe the canonical way of overriding with use-package while keeping other benefits is to add another (use-package... at the next location where you want to override things and adding a :defer t if the loading triggers already have been defined.

syl20bnr commented 9 years ago

The location to override use-package is before the initial use-package is evaluated. Not sure about this one but hooks are just variables right ? If the package is :disabled the hooks are not run.

syl20bnr commented 9 years ago

In scratch buffer I can do:

(add-hook 'foo 'bar)

And it works, but nobody will ever run this hooked function to foo variable.

Hooks are just a convention. So as I see it, it is very simple to add to use-package: it reduces to just agree on a naming convention and document it properly.

thomasf commented 9 years ago

You are also not telling emacs to byte compile 'bar

byte compiling this:


(use-package discopackage
  :disabled t
  :commands (disco-command))

(add-hook 'use-package-discopackage-hook
          #'(lambda () (disco-command)))

Produces this log:

In end of data:
disco.el:24:1:Warning: the function `disco-command' is not known to be
    defined.

This does not:

(use-package discopackage
  :disabled t
  :commands (disco-command)
  :config (disco-command)
  :init (disco-command))
syl20bnr commented 9 years ago

mmmmmh I'm not sure to understand what it has to do with preventing use-package to :disable a package, the only code that will be in use-package is a run-hook call.

In my case this is not byte-compiled, it is put in a init.el like file.

thomasf commented 9 years ago

No, it's about the content of the functions added to the hook. It's probable that the functions added to the hook refers to the symbols of the package in some way and if those are not declared there will be compiler warnings.

I think it's a feature creep towards something else that what use-package is now and as you say yourself, it's already quite complicated as a whole.

I am trying to argue from the point of how I understand the current design even if I haven't designed it and only contributed a few lines of code.

thomasf commented 9 years ago

@Silex Seems good, I'll think we should wait and see what John has to say about things now.

syl20bnr commented 9 years ago

@thomasf but this warning is in the use-package user space, how many people byte-compile their configuration ? Using the hooks are optional, if one cannot leave with byte-compilation warnings then it is possible to not use them.

Moreover it is still useful for setting variables (which is the use-case targeted as stated at the beginning of the thread).

config hooks if byte-compiled will produce the warnings for sure but it is fine to me, I don't byte-compile my init like a majority of emacs users.

thomasf commented 9 years ago

@syl20bnr

The byte compilation supporting features of use-package are specifically mentioned in the README. The :defines keyword is there to help with exactly this thing as well.

And yes, "The fine grained keywords are convenience that mess up the design and make it harder to understand." which is in part fine since the intent of use-package IS convenience and collecting things that belongs together under the same expression related to the package which from what I understand you are not doing anyways.

You could probably implement your "pre-init > init > post-init > package is auto-loaded > pre-config > config > post-config" cycle very easy without use-package since you're not really reaping a lot of other benefits from it anyways.

syl20bnr commented 9 years ago

Indeed, I could make my own and keep keywords argument only for the loading pipeline related stuff, make the macro stackable (designed from the ground up to be so, not sure it is the case with use-package) and move all the other keywords I called "fine grained" into macro utilities that can be used inside use-package at any step of the pipeline.

That's cool but I don't want to lock my users in...

thomasf commented 9 years ago

Just keep it simple and your users will be able to adapt to whatever they need if that situation arrives. I mean, as long as you are using package.el for the basic package management stuff it's not like your solution is likely to become more complicated or less of a lock in than use-package.

I hope I haven't come off as too much of a bastard here today. It's just that the longer this conversation has progressed the stronger I have felt that use-package might be wrong for your use case and I've tried to explain why I believe that. I'm not just butt hurt about having hooks in use-package something like that :)

YoungFrog commented 9 years ago

Since it seems we're discussing the design of use-package here, here's my own egoistic comment on the matter. Thanks @Silex for asking.

My own usage is rather limited and I never really understood how all the keywords relate to each other. I tend to use :config, :init, :pre-load or :pre-init until one actually works for what I want. I'm all for making things more clear.

I too think that "init" case (defer vs non-defer difference) is problematic. My needs for use-package are mostly :

And then I guess that this is it. I hear that some of you want many more things, and I don't have a strong opinion on those. Of course I'll be glad to help to the extent of my ability.

Silex commented 9 years ago

FWIW, I have requirements very close to the ones @YoungFrog has. I'd be willing to bet that most people actually have the same simple requirements (eval things before/after packages are loaded + few convenience helpers to setup load-path/bindings/autoloads).

syl20bnr commented 9 years ago

The helpers should be migrated to plain macros in order to be called at any step. If for some helpers their scope should be limited to init or config then their name should reflect this by including init or config suffixes.

syl20bnr commented 9 years ago

Bump ! :-)

I would like to hear about @jwiegley on this proposal.

I was thinking about this proposal and the temptation to develop my own use-package is an easy one but I don't want to do it, I truly think that use-package can fit the spacemacs case.

By the way thanks a lot @thomasf for taking the time to explain what the potential issues could be.

Here is why it may be OK to incorporate hooks (important reminder: I talk about plain hooks, not keywords) in use-package:

  1. In the :disabled t case mentioned by @thomasf. The warning would arise when one uses the #' notation, so for someone using the hooks (which won't be the lambda user) there is the possibility to silence the warning by removing #'. Moreover if one disable a package I guess that he can correlate easily between the warning and the explicitly disabled package.
  2. The hooks will be used essentially to override the variables and set some key bindings. It will likely be limited to those use cases because of lazy-loading.
  3. Stacking use-package calls is not a practical use case, since :disabled t a package would mean to update all the calls to use-package (remember that spacemacs users may have different use-package calls for the same package at different places (i.e. one in a dotfile and one in .emacs.d)).

The hooks are at the heart of the emacs configuration mechanism, it should not harm to get some:

For the sake of performance or something, the generation of those hooks could even be linked to a use-package configuration variable use-package-generate-hooks.

jwiegley commented 9 years ago

Ok, I think the large set of changes in 562b804ec83a693afc822290f74f0397da3be619 should resolve the complexities and confusion for most people.

There are now exactly three keywords used to provide configuration:

:init always happens before the package has loaded. The only time when the package can load before :init is during byte-compilation, where it is done to bring variables and functions into scope for the sake of the byte-compiler. Otherwise, when the use-package is actually evaluated (either the uncompiled version, or the compiled version), then :init will always run prior to the package being loaded. One other thing to note about :init is that it will execute even if the package is not available, since no load check has been made yet. This can be actually be helpful, or not, so if you want an existence check to happen before executing that code, you'll have to put one in your :init block.

:config happens after the package is loaded. If load has been deferred, then this code is deferred as well. If loading is not deferred, then it happens right after the load takes place. This is the same behavior as before.

:idle happens after both of these, and after a set amount of idle time has passed.

And that's it. This should be dead simple to explain, to document, and now the code handling these options in use-package.el is almost equally as simple to read, since the action of all three keywords happens within the span of 30 contiguous lines of code.

I see no need at the present time for :pre- or :post- keywords, unless we want to provide a hooking mechanism for external tools. If we do do that, then I'd prefer for those keywords to be internal only, and documented "off to the side" for other package authors. For most, the above three keywords should be all that you need (and in fact, I've only ever needed two of them).