Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a bugfix and various improvements #6

Closed
wants to merge 5 commits into from
Closed

a bugfix and various improvements #6

wants to merge 5 commits into from

Conversation

tarsius
Copy link
Contributor

@tarsius tarsius commented Mar 22, 2013

No description provided.

Previously `elisp-slime-nave-mode' was enabled in
`ielm-mode' (explicitly) and `emacs-lisp-mode' (because the arguments
DOC and INIT-VALUE were switched in the mode definition.

Also add variable `elisp-slime-nav-mode-lighter'.
@purcell
Copy link
Owner

purcell commented Mar 22, 2013

Thanks for all this! I've merged the commits, but dropped 438d700 -- I can't imagine a situation in which someone would install the package but not want it enabled.

@purcell purcell closed this Mar 22, 2013
@tarsius
Copy link
Contributor Author

tarsius commented Mar 22, 2013

Actually that was the bugfix...

`elisp-slime-nave-mode' ... because the arguments
DOC and INIT-VALUE were switched in the mode definition.

If you insist on having the mode enabled by loading the library at least fix that by explicitly using t as INIT-VALUE not by it being so because of a bug.

Still the mode should not be enabled by default. There is one reason: I installed it because it sounded interested but didn't have the time to look at it, ending up with a key that did something very strange (if you are not expecting it).

@purcell
Copy link
Owner

purcell commented Mar 22, 2013

Are you sure about the fix? The existing args appear to be in the right place according to the docs for define-minor-mode:

(define-minor-mode MODE DOC &optional INIT-VALUE LIGHTER KEYMAP &rest BODY)

->

(define-minor-mode elisp-slime-nav-mode
  "Enable Slime-style navigation of elisp symbols using M-. and M-,"
  nil " SliNav" elisp-slime-nav-mode-map)

Regarding enabling by default, I can understand your point, but now that package.el allows users to easily install/uninstall packages, it's not wholly unreasonable for packages to be enabled by default when installed. Plenty of packages similarly use autoloads to add entries to auto-mode-alist, and I personally support that behaviour when done carefully.

-Steve

@tarsius
Copy link
Contributor Author

tarsius commented Mar 22, 2013

Are you sure about the fix? The existing args appear to be in the
right place according to the docs for define-minor-mode:

You are right. oO Good thing you didn't merge it then...

Though you should update the usage instructions if you stick with this.

And adding elisp-slime-nav-mode-lighter (possibly as custom option)
would still be good so that it isn't necessary to use diminish to lose
the lighter.

Regarding enabling by default, I can understand your point, but now
that package.el allows users to easily install/uninstall packages,
it's not wholly unreasonable for packages to be enabled by default
when installed.

Not everyone uses package.el.

Plenty of packages similarly use autoloads to add entries to
auto-mode-alist, and I personally support that behaviour when done
carefully.

A compromise would be to drop the autoload, so users at least have to
require the package for it's mode's to become active.

I agree that installing a package using package.el should enable it's
modes (in most/many cases). But it is package.el which should do that
not libraries that are part of the package. A package installed using
apt-get is usually also "more ready" out of the box than one installed
from an upstream tarball or repo.

Doesn't package.el support having some post-install and/or on-load
hooks (which can then be defined in each package's description). I know
that el-get does. If package.el cannot do this then that's a bug: it
forces such autoloads onto everybody so that the (justified)
expectations of users of package.el are satisfied.

Are you actively encouraging upstreams to autoload adding minor-modes to
the appropriate hooks? If you do that I would appreciate it if you
stopped doing that and instead implemented post-install and on-load
phases in package.el.

@purcell
Copy link
Owner

purcell commented Mar 22, 2013

And adding elisp-slime-nav-mode-lighter (possibly as custom option)
would still be good so that it isn't necessary to use diminish to lose
the lighter.

Yes, I'll do that.

Doesn't package.el support having some post-install and/or on-load
hooks (which can then be defined in each package's description). I know
that el-get does. If package.el cannot do this then that's a bug: it
forces such autoloads onto everybody so that the (justified)
expectations of users of package.el are satisfied.

There's no such support in package.el. I suppose it might already be possible to add blocks of code to libraries that would only execute if the library was loaded from an installed package.

However, I'm not certain that this is a problem for package.el because I still don't believe the current approach of eager autoloading is automatically bad. Emacs itself offers a lot of functionality without needing explicit "require"s, so it's hard to say where the dividing line is.

I'm not actively encouraging authors to autoload minor modes into hooks, but I'm happy when I see that a major-mode author has at least autoloaded some reasonable auto-mode-alist entries.

@Wilfred
Copy link

Wilfred commented May 22, 2013

FWIW, I'd like to voice my support for not enabling by default. If I have a bug or performance issue, there is no longer anything in my .emacs.d/init.el that I can comment out to verify it's not elisp-slime-nav.

I'm also moving towards a literate programming approach to my configuration and it bothers me I have nowhere I can advertise document usage of elisp-slime-nav.

No big deal though.

@purcell
Copy link
Owner

purcell commented May 22, 2013

It's funny, because the arguments @tarsius made above have gradually been sinking into my brain, and I've concluded that I agree: I'll remove the code which auto-enables elisp-slime-nav.

purcell added a commit that referenced this pull request May 22, 2013
@tarsius
Copy link
Contributor Author

tarsius commented May 22, 2013

Victory! :-)

@dgutov
Copy link

dgutov commented May 22, 2013

FWIW, I've been quite happy that this mode enabled itself in autoloads. If you don't want it enabled, you probably don't want to use it either, better just uninstall it. Finding out which minor mode is overriding the M-. binding is trivial in this case.

@tarsius

I agree that installing a package using package.el should enable it's modes (in most/many cases). But it is package.el which should do that not libraries that are part of the package. A package installed using apt-get is usually also "more ready" out of the box than one installed from an upstream tarball or repo.

I don't understand, are you using some other package management solution that makes it harder to uninstall unwanted packages than package.el? Or is this a consideration borne out of using a custom Emacs distribution like prelude that bundles some packages you don't want to use?

@purcell
Copy link
Owner

purcell commented May 22, 2013

@dgutov Here's how I convinced myself of the validity of this policy: I asked myself, if elisp-slime-nav were bundled with a future version of Emacs, would I expect it to be auto-enabled? My clear answer was no, and I don't believe that external packages should work any differently from bundled packages.

@tarsius
Copy link
Contributor Author

tarsius commented May 22, 2013

Oh, why didn't I just celebrate quietly... :-)

Yes, I am using my own submodule-based vaporware package manager. But that's not the problem, uninstalling is easy.

The reason is... oh thanks @purcell, I couldn't have said it better.

@dgutov
Copy link

dgutov commented May 22, 2013

By that line of reasoning, you automatically cut out the argument "why don't you just uninnstall it?", because clearly you can't.

Personally, I'd rather most of the optional packages were moved from Emacs to GNU ELPA. Then each of them would be able to do more naughty things, and I'd be able to only keep some of them in my system.

@purcell
Copy link
Owner

purcell commented May 22, 2013

A package can become available (autoloads and all) on a system in all sorts of ways. It would be inappropriate to assume that it always becomes available as a result of user's explicit choice to download a package using package.el. Therefore availability and activation should be separate.

@purcell
Copy link
Owner

purcell commented May 22, 2013

@dgutov I agree that if everyone used package.el to manage all their code, it might make sense to tie installation and activation more closely together.

@tarsius
Copy link
Contributor Author

tarsius commented May 22, 2013

@purcell yes. The only thing I would add to that is that I think the activation should happen in the "package recipe" not in the libraries being installed. But I am repeating myself.

@tarsius
Copy link
Contributor Author

tarsius commented May 22, 2013

@dgutov was your last comment addressing me? I can uninstall packages, I said it was easy.

@dgutov
Copy link

dgutov commented May 22, 2013

@tarsius Sorry, no, it was addressed to @purcell, and the "you" was hypothetical, depicting a person who has installed an Emacs distribution with elisp-slime-nav bundled.

I don't see how using a homegrown solution rather than package.el makes things harder, though, requiring additional flexibility.

activation should happen in the "package recipe" not in the libraries being installed

It would make more sense conceptually, but are there situations when el-get initializes a package (makes the feature available), but doesn't run the recipe hooks?

@purcell

A package can become available (autoloads and all) on a system in all sorts of ways. It would be inappropriate to assume that it always becomes available as a result of user's explicit choice to download a package using package.el.

It's a good argument to make in abstract, and it applies well when we're dealing with libraries and packages that can serve multiple purposes. I just don't think it's the case here. Don't mind me, though, I can live with it either way.

@swsnr
Copy link
Contributor

swsnr commented May 23, 2013

@purcell I am with @dgutov. I was actually somewhat surprised to find this mode disabled after the latest update, and I don't think that it was change for the better. I can't help but wonder about this discussion.

This package is no library, after all. Other packages don't depend on it, it is not even of use to other packages, and it certainly won't install itself automagically, so how should it possibly end up in the user's Emacs without being explicitly and intentionally installed by her? How could she ever be surprised, let alone harmed by finding the mode being enabled by default?

But don't mind me, too, after all it's a trivial change.

@purcell
Copy link
Owner

purcell commented May 23, 2013

@lunaryorn As discussed above, a piece of code being available doesn't imply that the end user has explicitly asked for it to be enabled. Minor modes bundled with Emacs do not automatically enable themselves in applicable major modes (think checkdoc, for example).

If someone decided to bundle elisp-slime-nav into an APT package of "50 handy emacs libraries", then on systems where that package is installed, it would be undesirable if every user found himself with the mode auto-enabled in his elisp buffers.

Sorry for any inconvenience, but it's not without careful consideration.

@milkypostman
Copy link

I'm just going to leave this here: http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/dired-x.el#n32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants