Problems with skewer-css autoload

[purcell]

The current skewer-css code has an autoload which auto-enables skewer-css-mode via css-mode-hook. However, this also auto-enables skewer in derived modes, such as less-css-mode, which is not compatible with skewer-css-mode.

As a user, this is a pain to work around, because it involves either undoing the hook after skewer-css has loaded, or adding further hooks to turn it off afterwards.

Automatically changing mode hooks is not generally recommended, because of this type of issue; it's hard for an upstream author to foresee the exact needs of every user. Would you consider accepting a pull request which removes the (add-hook ...) code and adds it to the usage instructions?

Cheers,

-Steve

[skeeto]

Interesting, I didn't realize derived modes invoked their ancestor's hook. What exactly is the conflict? skewer-css-mode won't actually work, of course, but that shouldn't harm anything since none of its functions will be invoked. The only problem should be if you had one of the three skewer-css keybindings in the major mode's keymap, because minor mode keymaps get priority on lookups. Looking at less-css-mode this doesn't seem to be the case, though a user of less-css-mode might try to add these bindings only to have them masked by skewer-css-mode.

If you're using autoloads -- which is the case when installed through Melpa -- what's initially being added to css-mode-hook is the autoload function. This is the only place where skewer-css will automatically loaded. Shouldn't just removing this from the hook at any time fix it?

(remove-hook 'css-mode-hook 'skewer-css-mode)

If skewer-css hasn't been loaded yet, it will never get loaded. That is, unless you require/load it yourself or you invoke M-x skewer-css-mode. Loading skewer-css.el will add the hook again, which I'm guessing is the annoying part for you. But since you're not using it it shouldn't get loaded. If skewer-css has been loaded, then remove-hook should still take care of it because the hook will be completely gone for good.

One of my design goals for Skewer is zero configuration requred: everything should work smoothly out-of-the-box. No one should have to set anything up just to try it out. This means I don't want to have instructions like "After you install, you need to add these lines to your initialization file." Even more so, I don't want to change the behavior for the existing users, who after upating would suddenly find things broken (the minor mode no longer enabled by default), requiring they debug the problem and reconfigure for it.

As a middle ground, instead of putting the mode toggle directly on css-mode-hook, the function installed on the hook could be discriminating about the major mode. It would look something like this, though I'd probably give it a name so that it can easily be uninstalled.

(add-hook 'css-mode-hook
          (lambda ()
            (interactive)
            (when (eq major-mode 'css-mode)
              (skewer-css-mode))))

But before I do this I want to understand why a simple remove-hook won't work for users who want to disable part of Skewer.

[purcell]

Just in case I'm coming across as a grumpy ranter, let me first say that I totally understand the desire to make everything magically work out of the box. And let me also say that skewer-mode is an excellent piece of work, for which I am grateful.

Part of the reason for me filing this issue is that I have personally taken flak for doing too much autoloaded magic set-up in my own code, which irked me at the time, but these days I have come to agree that autoloaded set-up code is best limited to adding auto-mode-alist entries in most cases.

The key argument is that having a package available is not the same as wanting to enable it. It shouldn't be necessary for people to uninstall packages in order to disable optional functionality, so it's arguably better for users to opt in than opt out.

Here's the kicker: if skewer-mode were a package bundled with Emacs, I don't believe you would think it should be enabled by default in css-mode. The same rules should apply to downloaded packages, and as one of the MELPA maintainers, it's kinda my job to spread this view among package authors.

Hope that makes some sense! :-)

-Steve

[dgutov]

The issue of adding the hook in autoloads aside (I have no strong opinion either way), how will moving the hook to usage instructions help with less-css-mode? If a user wants to use skewer-css-mode with some .css files, they will add the hook manually, and it will cause the same problem in less-css-mode buffers.

[purcell]

@dgutov The user will then clearly be responsible for making things work properly and choosing trade-offs, rather than the author of this package.

For more related discussion, see purcell/elisp-slime-nav#6, in which I have been convinced to remove the auto-enabling code from my own elisp-slime-nav minor mode.

[dgutov]

user will then clearly be responsible for making things work properly and choosing trade-offs

That's clearly not a solution. If I tell them to put something in the init file, and it causes problems, it's still a bug (only now it's in documentation).

[purcell]

@dgutov I disagree. It would be a suggestion rather than a solution. The point is that it is customary for libaries to simply give the user a function he can use to enable optional functionality, and then he will be responsible for arranging to call it when appropriate for his needs.

[milkypostman]

does any minor mode that is part of Emacs automatically get enabled by hook?

[purcell]

@milkypostman Good question. I can't think of one.

[dgutov]

Do these count?

http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/gdb-mi.el#n2893
http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/url/url-handlers.el#n344
http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/dired-x.el#n300
http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/simple.el#n6177

But most of the time, why would they? Just like you can't uninstall a package included in Emacs, any major mode-specific functionality that's supposed to be "always on" can be incorporated in the same major mode, maybe hidden behind a variable.

[milkypostman]

i'll admit I'm a little split on this subject. I most definitely think that keybindings are one thing, but there is a nicety to having a minor mode automatically be initialized when the package in installed. I agree with @purcell's point in the linked post about elisp-slime-nav but I also think with the availability of package.el packages, it might make sense to design packages to have the default be for non-experts.

I think the problem we have is that we are experts but not all people are, and maybe there is a case to be made for having a variable, possibly package-auto-initialize or package-sane-defaults or package-expert-mode that needs to be set before calling (package-initialize) that every package uses to determine if it should initialize sane defaults or not.

for this package especially, it seems logical that if you don't want the package enabled you would just uninstall the package and restart emacs.

But I think that http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/dired-x.el#n32 goes to support that this package should not auto load itself at CSS time.

[purcell]

The simple (and arguably idiomatic) solution is for packages to provide an autoloaded foo-default-setup function, or even a foo-setup.el file. Then beginners can simply invoke that code, and experts can choose to do things differently if they prefer.

[skeeto]

I finally made up my mind and you've convinced me, Steve. What do you think of 4e4e1ec?

[purcell]

Yeah, that looks good! I definitely think this is the way to go, and the skewer-setup function provides the best of both worlds.

[milkypostman]

Is requiring the autoloads file correct here?

On Tuesday, May 28, 2013, Steve Purcell wrote:

Yeah, that looks good! I definitely think this is the way to go, and the
skewer-setup function provides the best of both worlds.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-18583105
.

[skeeto]

@milkypostman I figured maybe it's useful when used outside of package.el. The NOERROR argument is set so it shouldn't hurt anything.