general.el
general.el copied to clipboard
noctuid
Add way to define repeatable commands?
Hi there @noctuid , I'm a big fan of general, so let me first thank you for it. It makes defining keys in Emacs so simple and pleasant.
I came up with this today: https://github.com/alphapapa/defrepeater.el (Reddit thread). I was preparing to submit it to MELPA, but as someone commented, it reminds of Hydra, and it also seems related to what general does.
Would you be interested in integrating this into general in some way? I'd be glad to contribute a PR. It seems a bit of a shame to make a new package containing just this macro when it would seem to fit nicely in this package. It would be great if we could define the repeating command and bind the key at the same time!
Thanks.
Thanks for mentioning this package. I'd rather provide integration with packages like this than absorb them even if they are simple. I imagine that some people would find the functionality useful without wanting to use general.el, so I think it makes sense to keep it as a separate package. I've had this functionality requested before (#72). Hydra can be used to do this, but your package is much simpler and focused on this specific use case. It would be much easier to integrate it with general, and I think that would be great. @ambihelical defrepeater looks a lot more in line with what you wanted.
If you provided a macro that returned a new, automatically named function, the verbosity would be a little better (and that would work with any key definer):
(general-def key (defrepeater-create def))
As for direct integration with general, a :defrepeater keyword would likely be more convenient:
(general-def key '(def :defrepeater t))
;; or for all definitions
(general-def
:defrepeater t
key1 'def1
key2 'def2)
I'd like to add something like that to general, but it would require reworking the extension system first (e.g. extended definition keywords can't currently alter the definition). I've been planning on doing that anyway though.
Thanks for mentioning this package. I'd rather provide integration with packages like this than absorb them even if they are simple. I imagine that some people would find the functionality useful without wanting to use general.el, so I think it makes sense to keep it as a separate package.
That makes sense.
Hydra can be used to do this, but your package is much simpler and focused on this specific use case.
I tried implementing the winner-mode example with Hydra, and it worked so well that I wondered if there is still any reason to use defrepeater. I guess it could still be useful, especially if it could be integrated with general.
If you provided a macro that returned a new, automatically named function, the verbosity would be a little better (and that would work with any key definer):
(general-def key (defrepeater-create def))
Oh, you mean instead of:
(defrepeater 'winner-undo-repeat #'winner-undo)
You would like:
(defrepeater #'winner-undo) ;; => (function winner-undo-repeat)
Or something like that? That's not a bad idea. The first macro I made was define-repeatable-command, which was like defun but created two functions, one to do the body forms and one to repeat it. But I realized that defining only the repeating part is more flexible, and then I decided to imitate defalias.
What if I made the new function name optional? e.g. these would do the same thing:
(defrepeater 'winner-undo-repeat #'winner-undo)
(defrepeater #'winner-undo)
Thanks for your help.
Using hydra is a little more involved, and some people may not need/want its full functionality. I'm fine with implementing repeating in general using either with hydra or with defrepeater. I don't think it matters too much which is used, but @ambihelical did have some annoyances with hydra for this (see #72). If you want to make a PR, I could let you know when the extension system is capable of doing this. It should be pretty easy to add support for then. Otherwise, I'll get around to doing it eventually.
What if I reversed the argument order and made the new function name optional? e.g. these would do the same thing
I think that would be pretty convenient.
Ok, sounds great. Let me know when it's ready and I'll work on the PR. And I'll go ahead and submit to MELPA soon. Thanks.
BTW, completely off-topic, but I noticed the extensive table of contents in the readme, and thought you might find this new package useful to help customize it: https://github.com/alphapapa/org-make-toc
BTW again, if I may misuse this issue to ask a small question, just curious: where did the awesome logo in the readme come from? :)
BTW, completely off-topic, but I noticed the extensive table of contents in the readme, and thought you might find this new package useful to help customize it: https://github.com/alphapapa/org-make-toc
I'm using toc-org for it right now. I wasn't aware of your package when I started using it but saw you mention it on reddit. I will try it out. Maybe you could provide a comparison in the readme?
BTW again, if I may misuse this issue to ask a small question, just curious: where did the awesome logo in the readme come from? :)
It's taken from the SRPG Advance Wars Days of Ruin.
Ok, sounds great. Let me know when it's ready and I'll work on the PR. And I'll go ahead and submit to MELPA soon. Thanks.
Alright. I fixed the extension system so that it can be used to alter definitions. Basically all you'd need to do is add :repeater to general-rewrite-def-keywords and create a function called general-extended-def-:repeater that created the repeater command and returned a new extended definition plist with :def as the new command. See general-extended-def-:predicate for a basic example of a function that alters the definition and see here for the full documentation on the extension system.
I'm using toc-org for it right now. I wasn't aware of your package when I started using it but saw you mention it on reddit. I will try it out. Maybe you could provide a comparison in the readme?
I just published it, actually. The primary differences are explained in the "Advanced" section of the readme, although they are not mentioned as a comparison to toc-org:
A document may contain multiple tables of contents. Tables of contents can be customized by setting the TOC property of headings to these values:
all: Include all headings in the file, except ignored headings. children: Include only child headings of this ToC. siblings: Include only sibling headings of this ToC. ignore: Omit a heading from the TOC. ignore-children or 0: Omit a heading’s child headings from the TOC. a number N: Include child headings no more than N levels deep in the TOC.
toc-org is a fine package, and will meet the needs of most users, so you may not need to switch. But if you need some of these features, or if you would prefer to use Org properties instead of tags to control the TOC, you could use org-make-toc instead.
It's taken from the SRPG Advance Wars Days of Ruin.
Cool, looks like a lot of neat character art around that game.
Alright. I fixed the extension system so that it can be used to alter definitions. Basically all you'd need to do is add :repeater to general-rewrite-def-keywords and create a function called general-extended-def-:repeater that created the repeater command and returned a new extended definition plist with :def as the new command. See general-extended-def-:predicate for a basic example of a function that alters the definition and see here for the full documentation on the extension system.
Ok, this may take me a little while... :) Thanks.
toc-org is a fine package, and will meet the needs of most users, so you may not need to switch. But if you need some of these features, or if you would prefer to use Org properties instead of tags to control the TOC, you could use org-make-toc instead.
I will look into whether it would be a better fit for this package's readme when I get the chance.
Ok, this may take me a little while... :) Thanks.
I'd be glad to add support myself if you'd like. Is the syntax of defrepeater finalized?
I'd be glad to add support myself if you'd like. Is the syntax of defrepeater finalized?
That would be good, if you don't mind. I have several other projects I'm working on, and general's inner workings are more than I feel like diving into right now. :)
I think the syntax is finalized, yes, but the package isn't on MELPA yet. I'll work on that now.
Okay, I'll add it once the package is on Melpa (general won't have defrepeater as a direct dependency but having it on Melpa makes it easier/possible to test with cask).
I also have a couple of suggestions for defrepeater. defun's return value is technically undefined (and while it is unlikely, it could potentially change, so defrepeater should explicitly return the created function.
Also, while it's unlikely that there will be any problems, you might want to prefix the new command with defrepeater- to avoid the possibility of name clashes.
defun's return value is technically undefined (and while it is unlikely, it could potentially change, so defrepeater should explicitly return the created function.
I see that the docstring for defun indeed says that, but I'm not sure what the point of returning the function is. The macro is simply a wrapper around defun. defrepeater isn't intended to return a certain value. This is not to say that I won't change it, but is there a need? Were you planning to use its return value? (I guess it would save you from munging the symbol name?)
Also, while it's unlikely that there will be any problems, you might want to prefix the new command with defrepeater- to avoid the possibility of name clashes.
The command is suffixed with -repeat, and the macro ensures that the function is not already defined. I think I prefer that scheme over prefixing the created commands with defrepeater, because the commands do not define repeaters, and using a suffix means that lists of commands sort alphabetically. What do you think?
Thanks for your feedback.
but I'm not sure what the point of returning the function is.
It's useful to immediately bind the created function (and would also simplify implementing a :repeater keyword). For example, (define-key <keymap> <key> (defrepeater <def>)) would break if defun did not return the function. In my macros that create a function, I just have something like #',name at the end. It's kind of ridiculous that the return value isn't guaranteed, but I prefer to be safe.
because the commands do not define repeaters, and using a suffix means that lists of commands sort alphabetically
I definitely agree that the naming scheme makes more sense. I don't like the idea of the automatic naming ever failing though. Also, it would probably be better if there was no user-error if a defrepeater was run twice (e.g. reloading config). Maybe use defrepeater-<command> and make an alias for <command>-repeat if possible? The package could also potentially be renamed so that the prefix makes more sense. Feel free to do whatever you want if you think this is overkill; it is a rare case.
It's useful to immediately bind the created function (and would also simplify implementing a :repeater keyword). For example, (define-key
(defrepeater )) would break if defun did not return the function. In my macros that create a function, I just have something like #',name at the end. It's kind of ridiculous that the return value isn't guaranteed, but I prefer to be safe.
Got it. I'll add the return value. Thanks.
I don't like the idea of the automatic naming ever failing though.
I thought about adding a force argument, but I'm not sure if that's a good UI. I'm not sure if removing the check would be a good idea, though; I feel like defrepeater should be safe, e.g. it should never redefine a function by accident. What do you think?
Also, it would probably be better if there was no user-error if a defrepeater was run twice (e.g. reloading config).
You're probably right. Do you think a message or warn would be better?
Maybe use defrepeater-
and make an alias for -repeat if possible?
Hmm...maybe. I feel like the defrepeater prefix should be reserved for functions/macros defined in defrepeater.el, not anything defined by using defrepeater. But I'm not completely sure.
The package could also potentially be renamed so that the prefix makes more sense.
Can you give me an example of what you mean? (repeat is already taken, BTW, it's built-in.)
Thanks.
I agree that functions shouldn't be redefined, but I think that it would be preferable if there was no possibility for collision/no reason to warn or message. Prefixing created functions with the prefix reserved for the package would reasonably ensure that. It's fairly common for function/variable-defining macros to prefix with the package prefix.
As for what prefix to use, I was thinking repeater if it isn't already taken. hydra defines defhydra even though its prefix is hydra-. I don't know if that's considered a poor practice, but it's another possibility. repeater-def isn't that bad either.
If you don't want to change the prefix, I'd think it would be fine to leave it as a warn.
Ok, I think I'll use warn. I'd prefer to leave the prefix the way it is for now. Thanks for all your feedback.
Now that it's in melpa, I will add a :repeater keyword. Based on the syntax, I was under the mistaken impression that it would support a variable for the first argument. If you use eval instead cadr, this will be possible. Otherwise, I will need to use eval in general (I'm not aware of any alternatives). I think it would be preferable to use eval when the macro expands instead of having to call eval on a backquoted macro. Which would you prefer?
Hi again,
I understand and share your reluctance to use eval, but I feel like it would be a bad idea for me to eval arguments passed to defrepeater. I feel like it's more acceptable and expected for general to do the eval, since it's intentionally wrapping macros in other macros. Does that make sense? Of course, you have more experience in this area than I do, so I may be mistaken.
Thanks.
I don't think that it is much of an issue for defrepeater to use eval for this. One of us has to use eval with defrepeater in its current form. General using eval isn't really any better and is possibly worse. Eval would end up being called from a function and not a macro. I'd have to do something like this in general:
(eval `(defrepeater ,def))
The issue with this is that the macro won't actually be expanded until the function is called. This isn't a huge deal in this specific case (performance almost certainly won't be an issue), but generally you want to macros to be expandable during compilation. If you find yourself needing to eval a macro (e.g. for the above reason, because you need to use apply, etc.), it would probably be better to use a function.
Since defrepeater wraps defun, it currently needs to be a macro. That said, one potential solution that doesn't involve eval would be to use something like (setf (symbol-function symbol) ...) instead of defun. I need to look at elisp's defun to see if it does anything extra that is necessary, but would you be okay with potentially creating a function version of defrepeater if possible? Generally macro definers have unqouted names, but defrepeater could either be left alone (if you'd prefer) or have a new syntax like (defrepeater winner-undo).
I don't think that it is much of an issue for defrepeater to use eval for this. One of us has to use eval with defrepeater in its current form. General using eval isn't really any better and is possibly worse. Eval would end up being called from a function and not a macro. I'd have to do something like this in general:
Hmm. Well, I guess I've been thinking of this in terms of use-package keywords, which I think of as wrapping existing, independent code on its own terms. Also, I think defrepeater should use syntax like defalias. From that perspective, I don't think it makes sense for defrepeater to call eval. I think of general as "getting its hands dirty" behind the scenes, doing whatever it takes to make things nice for the user, which could include calling eval.
The issue with this is that the macro won't actually be expanded until the function is called. This isn't a huge deal in this specific case (performance almost certainly won't be an issue), but generally you want to macros to be expandable during compilation. If you find yourself needing to eval a macro (e.g. for the above reason, because you need to use apply, etc.), it would probably be better to use a function.
I guess the issue here is that I don't find myself needing to eval a macro. :) I very much appreciate what general does, and it's not that I'm unwilling to accommodate it to a certain extent, but were it not for general, this wouldn't be an issue. I feel like it's general's "responsibility" to handle this, and that defrepeater should be independent and not use eval unless it needs to for its own sake.
I need to look at elisp's defun to see if it does anything extra that is necessary
From a quick glance at it myself, it essentially wraps (defalias (quote name) (function (lambda ..., handling declare forms, which defrepeater doesn't use. Maybe we could build the defalias call directly. On the other hand, I guess that would make it less forward-compatible with future changes to defun (which are probably unlikely, but still...).
would you be okay with potentially creating a function version of defrepeater if possible?
Maybe, but I feel like I need to understand it better before changing it.
Or did you mean an alternative, coexisting version, rather than replacing the macro version? That would probably be okay, but I want to be sure it's necessary before adding it.
Generally macro definers have unqouted names, but defrepeater could either be left alone (if you'd prefer) or have a new syntax like (defrepeater winner-undo).
I think defrepeater should emulate defalias's syntax, so I'd rather not change that.
Thanks.
should use syntax like defalias
Well defalias does normally use fset. For example, this works:
(defvar var #'foo)
(defalias var #'+)
(foo 1 2)
;; => 3
On the other hand, this won't work with defrepeater.
I guess the issue here is that I don't find myself needing to eval a macro. :) I very much appreciate what general does, and it's not that I'm unwilling to accommodate it to a certain extent, but were it not for general, this wouldn't be an issue.
This is really an issue for any function that wants to call defrepeater with a variable, not just general. Given that defrepeater has syntax like it was function, I don't think it's unreasonable to expect it to evaluate its arguments like a function. For example, if a user had a list of functions they wanted to define/bind repeatable versions for (with dolist or however), they would also have to use eval. Maybe this is unlikely, but it's a possibility.
Or did you mean an alternative, coexisting version, rather than replacing the macro version? That would probably be okay, but I want to be sure it's necessary before adding it.
Yes, that's what I meant. I have no issue with you leaving defrepeater as is. It's not necessary, but a function equivalent would mean that general (or a user) wouldn't need to use eval. I could potentially make a PR if you want. If you'd rather not do this, I'll just use eval.
Actually, you're correct about defrepeater using eval; my bad. You can't rely on argument values during macro expansion. You can if the variables are defined at expansion time, but that's not the case here.
The only reliable ways to still generate a sanely named function would be to add an additional function definer that generated the name at runtime or for general to use eval to expand defrepeater at runtime. If there was a helper function that dynamically generated the name, defrepeater could expand to it instead if the argument(s) passed in are unqouted. That would allow defrepeater to work the same as defalias for the above example. My apologies again for the initial incorrect suggestion.
I'll make a PR for this soon, or if you'd rather not add this complexity, I can leave defrepeater as-is. I think it would be nice to be able to use variables with defrepeater though.