Enable advices only when llama fontification is enabled #9
Conversation
Do not install advices if llama.el is loaded but Llama-specific fontification is not enabled. This reduces the impact of the advices for most users, who have loaded a package like Magit which requires Llama but who do not edit Elisp files with Llama-expressions.
|
Another option could be to always activate all the advices and even the fontification by default as soon as llama.el is loaded (maybe even remove the special fontification mode). Then require llama like this in other packages such that it is only loaded for the macro during compilation: ;; in packages only for the macro
(eval-when-compile (require 'llama))Users who edit files with Llama expressions can then simply require in their init.el: ;; init.el
(require 'llama) |
The additions to font-lock and the advices are not only useful to people contributing to libraries that use They also help users who just happen to run into some The
|
|
I see. So you never want to degrade editing functionality while fontification should be separate? What do you think of a customization option fort this? There could be a (defcustom llama-fontify nil
"Should llama-mode additionally fontify ##?"
:type 'boolean)
;; Turns on basic advices for completion
;; replaces llama-fontify-mode, enable llama-mode and use llama-fontify=t instead
(define-minor-mode llama-mode ...)My point here is that I am generally trying to reduce advices as much as possible to prevent side effects and breakage, guarding them behind a mode. Otoh the llama advices are probably quite harmless, like the simple all-completions advice. I hope that some llama-like syntax gets adopted upstream at some point, such that the problems, which you work around with advices, are avoided in the first place. |
Yes, I think it is very important to address all issues that arise from the use of
I think this would increase fragility. I see no benefits beside making sure we do the-right-thing™. It certainly would increase the already surprisingly high complexity.
I was wondering what the motivation was. I agree that it is generally good to avoid advices, but believe their use is appropriate here. We should reduce the risk of breakage, but I believe that making the advices optional achieves the opposite. (It might be possible to get the changes to
I am glad you are in this camp too. Until the real syntax arrives, this package has to fake it. My goal with this package is to give people a chance to realize that such syntax is nice, but that what they really want is the real thing. If users run into the issues these advices address, that would jeopardize that goal. Likewise a minimal-viable version of |
I think I don't need the advices as long as I don't actually edit code which uses llama. This means I would not observe any degradation, while at the same time I wouldn't have to worry about issues due to the advices.
Well, something which is not there will not cause issues. My point is that the majority of llama users uses llama indirectly via other packages, but does not edit code which uses llama. That's the main motivation - to reduce the impact and risk. I see your point about fragility - as soon as you edit code with llamas the degradation will become apparent. However this only affects a fraction of the developers while the advices are installed for everyone.
Which edge cases and complexity do you mean - the syntax problems due to ##? The macro itself should be relatively straightforward. So as long as someone uses llama with the non-special syntax, i.e., |
|
To clarify -- it's supposed to be on by default?
It is not what I observe, when I take |
Sight. Yeah, I though this was enabled by default but it is not. IMO it should be enabled by default but apparently when it came to making that decision, I went with what I felt was "safe" and played by the rules, instead of doing what I felt was right. |
|
@minad How strongly do you feel about this? Can I just say no, or does that make me stubborn and impossible to reason with? |
|
To chip in, though you two are wiser than I:
|
|
The fontification mode is not enabled because the manual says: Beside being a no-no, enabling a minor-mode by default also does not work properly without additional work, see all the hackery necessary to enable |
Of course you can say no. I feel pretty strongly since it seems to me that things could be handled more cleanly, but I often feel pretty strongly about unimportant Elisp details, so this should not mean much. In any case, I wouldn't feel bad about it if you say no and just close this. Generally I prefer if only the minimal functionality is enabled to make something work. As I argue, most users only need the macro, the all-completions advice and nothing else as long as they don't edit llama-heavy code. I follow such general principles in the packages I maintain, e.g., Compat where we try hard to avoid any kind of advice. It may also help adoption, if the package is less intrusive - at least I feel more comfortable adopting packages which avoid sprinkling multiple advices in some obscure areas. For me less is more. Maybe that's a bit odd given that I do a lot of stuff in Emacs and Emacs is not known for trying to be minimal. :)
Yes, indeed. However this only makes sense if most users actually need the advices. I argue that this is not the case, given that the majority of users does not write much Elisp and uses llama only as an indirect dependency. So you are battle testing a feature which most users do not need against unassuming users, instead of only against the group of experienced developers which enable the feature knowingly (as this PR proposes).
That's what I would do - I would grep the Emacs code base and the ELPA directory of installed packages. This works almost always, except if someone actively tries to obfuscate a symbol. (EDIT: I tried this. While I found in the Emacs code base that ## means empty symbol, there is lots and lots of noise since # is used for comments in shell scripts, makes files and in the C preprocessor.)
The question is if many users actually run across the ##. But independent of if fontification and advices are enabled by default, the motivation behind this PR here was also that the advices and fontification are not handled consistently. There is no way to turn some of the advices off, while fontification is off by default. As I proposed in my comment above, a |
|
FWIW, you can find my (unfinished) implementation of this in the |
|
@tarsius I don't see the branch. Maybe you didn't push it yet? I can take a look later. |
|
I have pushed that branch now. (And will delete it again in a few days.) I have decided to stick to the status quo. I believe the alternatives only increase safety in theory. They decrease safety and increase complexity and maintenance cost. |
|
Actually, I might still bang on it some more. But there are other things I want/need to work on first. On the other hand, my mind is now preoccupied with this, so I might have to power through, to be able to concentrate on other things again. |
|
I've pushed a new iteration to |
|
Looks good to me! Also the global fontification mode is a nice simplification. |
|
I like it too. :) Appreciating the attention to detail. Thanks for figuring out all this! |
|
Pushed after some more tweaking. |
Hello Jonas,
as far as I understand most of the advices are only needed when editing files with Llama-expressions, such that we could only enable the advices when the font locking mode is requested. There are many more users who load llama.el, than the ones who edit such files. Do you think this is a good idea?