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

lispy core #74

Closed
tuhdo opened this issue May 30, 2015 · 39 comments
Closed

lispy core #74

tuhdo opened this issue May 30, 2015 · 39 comments

Comments

@tuhdo
Copy link

tuhdo commented May 30, 2015

Is it possible to make lispy just lispy, with the exception of avy included for ace stuffs? I don't use ivy, but lispy includes it. Some people may not use hydra as well, so it should not be forced.

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

I don't use ivy, but lispy includes it. Some people may not use hydra as well, so it should not be forced.

Those packages need to be included because there are no core packages to fall back on. Lispy used to depend on helm when it was the only package capable to provide sufficient completion. Now ivy provides that as well, and I've made it the default. There's no way to specify a dependency like:

You must have ivy or helm installed or else the core features won't work

Same thing with ace-jump-mode/avy: they're required to make the core features work.

@abo-abo abo-abo closed this as completed May 30, 2015
@tuhdo
Copy link
Author

tuhdo commented May 30, 2015

I think the core should be about manipulating sexp/parens without external dependencies. Other fancy stuffs, if a user uses one, should print a message that it requires a dependency. It's better that way instead of installing several packages at once.

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

I think the core should be about manipulating sexp/parens without external dependencies.

I disagree, it's all about the fancy stuff for me:

  • ace-window is required for debugging, which is the core of the package.
  • hydra could be made optional, but it would be very ugly. Besides, I plan to make x use it at some point.
  • iedit is required for xb, and is generally a useful thing to have.
  • multiple-cursors could actually be made optional.
  • swiper is essential for tags navigation: I don't want a first time user to type g and think it's crap because of completing-read under-performing.

@tuhdo
Copy link
Author

tuhdo commented May 30, 2015

So, how about making swiper optional if a user already has Helm and make Lispy use Helm by default?

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

So, how about making swiper optional if a user already has Helm and make Lispy use Helm by default?

I'd gladly do it if package.el had that capability. But it doesn't, so I don't.

@tuhdo
Copy link
Author

tuhdo commented May 30, 2015

Ok, but originally I suggested that if a user uses a fancy feature and the required dependency is not available, lispy prints a message telling him to install the dependency. I think it's a good thing since the user knows the existence of such package.

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

a fancy feature

Jumping to tags and occur aren't fancy features, they are core features.

CIDER, SLIME and Geiser are fancy. Lispy cooperates with them without installing them.

What you're suggesting should be solved on the level of package.el and not lispy.el and any other package. Something like:

(define-package "lispy" "0.24.0"
  "vi-like Paredit"
  '((emacs "24.1")
    (ace-window "0.8.0")
    (hydra "0.12.1")
    (iedit "0.97")
    (weak (multiple-cursors "1.3.0"))
    (or (swiper "0.2.0") (helm "1.5.5"))
    (recommend (cider "1.0.0"))
    (recommend (slime "1.0.0"))
    (recommend (geiser "1.0.0"))))

Here, weak means a weak dependency: install it initially, but don't re-install it if the user removes it. And don't complain if it's not present. Other dependencies are strong. or means at least one of the dependencies should be present. recommend is just a comment in some menu for packages that could be useful, it won't be auto-installed.

Maybe @Malabarba could comment if these style of dependencies are possible or useful.

@Malabarba
Copy link

I think they're both possible and useful. The only problem right now is that (if this were to be implemented) a package which specifies dependencies like this would not be compatible with the current package.el version, so user of Emacs < 25 wouldn't be able to install the package.

In order to avoid that, and to allow packages to work on all package.el versions, the syntax for specifying these special dependencies would have to be designed in a very clever way.

@Malabarba
Copy link

And if I may add to the actual discussion: I think the disagreement comes from the fact that lispy has gradually evolved so much that it is not really "vi-like Paredit". Paredit is really just a score of movement and text-manipulation commands.

I'm not saying all these lispy features are just "fancy stuff" (in fact, I feel these features create a cohesive development environment and I like when things just-work).
I'm just saying maybe lispy is overdue for a new description. :-)

As for @tuhdo's requests, you could then refactor the manipulation of sexps/parens into a separate package, and use that as a dependency for lispy. Then people who just want the very basic stuff could just install that. However, I have no knowledge of how lispy works internally, so this might just be more trouble than it's worth.

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

more trouble than it's worth.

Exactly. I prefer adding features to just shuffling code about. I start shuffling only if the code become overwhelming, which it isn't at the moment.

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

I'm just saying maybe lispy is overdue for a new description. :-)

Probably. I don't want look arrogant by using the term IDE: lispy doesn't compare to SLIME. Maybe "vi-like LISP Toolkit".

@tuhdo
Copy link
Author

tuhdo commented May 30, 2015

What @Malabarba suggested is reasonable: you can make a new Lispy minimal package with no external dependencies. Then, when a user incrementally learns Lispy, he may encounter a feature that requires an external package. In this case, Lispy prints a message suggesting him to install the dependency to use this feature. I think making a package out of existing package should be easy for you.

@abo-abo
Copy link
Owner

abo-abo commented May 30, 2015

Then, when a user incrementally learns Lispy, he may encounter a feature that requires an external package

This never works. I joined Emacs before package.el, and I hated having to go through a lengthy README just to install a package.

I think making a package out of existing package should be easy for you.

It's not easy. Having everything in one place means I can use lispy-goto and git grep easily. I don't see why I should give up ease of development for a dubious user convenience. You're a big fan of helm: it goes with the same way of keeping all things in one place which I like.

@Malabarba
Copy link

I agree the priority should be having features ready out of the box. I know some people like to keep things as slim as possible, but I think for most people their time is more important than their disk space.

@tuhdo
Copy link
Author

tuhdo commented Jun 9, 2015

Ok, so here is the idea: how about separate Lispy commands that depends on nothing into a file, and commands that depend on things another files, then we can introduce a package on MELPA with such Lispy core package. I want such a package, so I can integrate it into Spacemacs, since it already has its replacement for Hydra and Ivy.

@abo-abo
Copy link
Owner

abo-abo commented Jun 9, 2015

Ok, so here is the idea: how about separate Lispy commands that depends on nothing into a file

It's possible to do, but I'm not a big fan of shuffling code just for the sake of it. Is it so objectionable to you that lispy pulls in two packages that you don't need to use if you don't want to? Two extra packages at ~3000 lines extra, ivy is autoloaded, only hydra is required because of byte compilation. ~1000 lines of always loaded hydra code doesn't compare to tens of thousands of always loaded CEDET code. Even to ~7000 lines of always loaded lispy.

The venerable evil package does a bigger crime than that: it not only pulls in undo-tree but auto-enables it everywhere even if I turn off evil, even before evil-mode is activated for the first time. Yet there are zero complaints and this behavior remains unchanged for years.

@tuhdo
Copy link
Author

tuhdo commented Jun 9, 2015

The problem is if lispy as a whole is introduced to the core, it increases startup time which is a strength of Spacemacs, since it loads over 130 packages within 1.7 seconds.

tens of thousands of always loaded CEDET code

I don't think CEDET is loaded if you don't enable it. And in Spacemacs, CEDET is lazy loaded as well.

The evil package does it for a good reason: instead of reinventing the way Vim does undo, it reuses an existing package. And yes, to implement it needs that much code. And only Vim users use Evil anyway, and they will complain without such package emulating Vim behavior.

In the case of Spacemacs, it already has Helm and Ido and Evil micro state (that gives the same feature as hydra), so adding hydra and ivy is redundant and no one uses it, since most effort goes into Helm and Ido already. Spacemacs would love to have worf as well: syl20bnr/spacemacs#1417 (comment), unfortunately it depends on hydra so it's hard to add. If it can be add, Spacemacs users may add support worf support for Evil and push back here, so you can even get wider audience.

@abo-abo
Copy link
Owner

abo-abo commented Jun 9, 2015

I don't think CEDET is loaded if you don't enable it

It is fully loaded if you (require 'lispy).

The problem is if lispy as a whole is introduced to the core, it increases startup time

By how much? My config starts in 1.3 currently, I have 130 packages, and lispy isn't lazy-loaded.

The evil package does it for a good reason: instead of reinventing the way Vim does undo, it reuses an existing package

Reusing a package wasn't my complaint. Auto-enabling it even when evil isn't on was my complaint.

so adding hydra and ivy is redundant and no one uses it, since most effort goes into Helm and Ido already

So there's place for two completion packages but not three? That's nonsense. Especially considering that helm is 31000 lines and takes ages to update, while swiper is 2000 lines. And what to do with smartparens after lispy is added? It becomes redundant, doesn't it?

Bundling code and loading it are two different things. I don't see spacemacs people complaining on emacs-devel that emacs bundles 1500 el files.

Spacemacs would love to have worf as well
unfortunately it depends on hydra so it's hard to add

Worf is experimental and hasn't had a single release. I can tag a pre-hydra version in MELPA stable if you really want. I'm not extending that package lately.

@abo-abo
Copy link
Owner

abo-abo commented Jun 9, 2015

In any case, making lispy-core.el isn't a bad idea and it will be done eventually. If you're willing to maintain a separate MELPA recipe for lispy-core, I don't mind to have a package header in that file. But the whole lispy package should still be there, come without gimped completion, and not depend on lispy-core (I don't really care for sending a PR to MELPA each time I want to add a new file). When package.el provides an option for alternatives, lispy will depend on (or 'ivy 'helm).

I can work on removing the dependency on multiple-cursors and making sure that hydra-dependent things still compile when hydra isn't present. But it's possible that x will switch to hydra, in which case it becomes essential.

@tuhdo
Copy link
Author

tuhdo commented Jun 10, 2015

By how much? My config starts in 1.3 currently, I have 130 packages, and lispy isn't lazy-loaded.

I remember reading a post where you autoloaded everything. Lazy-load with use-package is autoload. I checked it again. Actually stock Spacemacs without any layer starts within 1.5 second. It is because Evil and some other packages must be fully loaded in the start. When I add more layers the startup time increase.

Auto-enabling it even when evil isn't on was my complaint.

If so, you should not install it in the first place.

So there's place for two completion packages but not three? That's nonsense. Especially considering that helm is 31000 lines and takes ages to update, while swiper is 2000 lines. And what to do with smartparens after lispy is added? It becomes redundant, doesn't it?

Actually Ido is built-in so we cannot do anything about it. And the killer thing about Ido is flx-ido. I think if you can integrate ivy with Ido, it will give people stronger incentive to switch to ivy. flx-ido is just too convenient.

I'm not sure about this, so I may ask @syl20bnr whether it's ok to add such package. However, I am certain about one thing: currently Helm is only package that can compete with Vim packages and win. No other narrowing and completion package in Emacs can compete with the likes of Command-T or CtrlP or Unite, in term of speed and scalability. Ido with flx-ido is nice, unfortunately it only works for small number of candidates. To satisfy the demands from Vim users, Helm is essential.

About lispy, I believe it's the most convenient editing package out there, in term of ergonomic and feature. I really wish it was to be integrated to Spacemacs. @syl20bnr what do you think when we have lispy-core?

In any case, making lispy-core.el isn't a bad idea and it will be done eventually.

Thank for you consideration! Sure, I will help adding and maintaining it. And I agree, the lispy as a whole should still be there.

But it's possible that x will switch to hydra, in which case it becomes essential.

Currently x works fine without hydra. So I think if a user does not install hydra, it should be using the current way we are doing.

@Malabarba
Copy link

hydra is required because of byte compilation.

Sounds like you could require it inside eval-when-compile.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

Sounds like you could require it inside eval-when-compile.

Thanks, I guess that could work. It could be possible to not depend on hydra, dispatch on featurep inside eval-when-compile, and generate a no-op or a warning lambda in case the package isn't installed. I can try that.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

Actually Ido is built-in so we cannot do anything about it.

Other than installing two packages that extend Ido.

And the killer thing about Ido is flx-ido.

flx-ido is slow, I avoid it on purpose. I could try to enable it for small collections, but then it would feel weird to have two different behaviors.

I think if you can integrate ivy with Ido, it will give people stronger incentive to switch to ivy.
flx-ido is just too convenient.

Ivy has started as an alternative to Ido, because Ido code was so convoluted that I couldn't extend it for my needs. In only 2000 lines, Ivy has alternatives to: ido-vertical, ido-ubiqiutous, helm-swoop, helm-git-grep and more. You should really check out counsel-git-grep, you've commented on the async behavior before and I've made improvements, maybe I can get more suggestions. I think it's better than helm-git-grep now, since it doesn't have screen tearing.

Helm is only package that can compete with Vim packages and win

Ivy is just as fast as Helm, some people may prefer it since it's smaller and easier to understand. For users coming from Vim, Ivy is basically a theme for Helm. Here's a quote from a happy user:

Ivy meets all my helm needs, while feeling faster

@tuhdo
Copy link
Author

tuhdo commented Jun 10, 2015

Ivy is not yet a replacement for Helm. For a Vim user:

  • Fuzzy matching is a nom in Vim world, with various solutions.
  • Have you tried to make Ivy work with my sample test of 250k files? If not, then Ivy is still not that fast.
  • Ivy does not have action list that allows multipole actions on a single candidate.
  • Many built-in Helm commands are useful that I cannot live without it:
    • helm-colors: help me find a face and customize it quickly.
    • Various helm-info-*commands: it's a joy to use this to quickly search for information.
    • helm-locate-library: help me quickly jump to a loaded Elisp file.
    • helm-locate: why should I need to browse to root directory to open any file? Just use the command and fuzzy match it, then open right here. No browsing needed.
    • helm-pp-bookmarks
    • helm-recentf
  • Helm has more useful packages supported it. For example: helm-descbinds, helm-ag/pt (it's so fast and it works on Windows), helm-gtags, helm-themes....
  • The ability to combine multiple sources.

@Malabarba
Copy link

Thanks, I guess that could work. It could be possible to not depend on hydra, dispatch on featurep inside eval-when-compile, and generate a no-op or a warning lambda in case the package isn't installed. I can try that.

As a general thing, if you only use macros from a package (regardless of whether the dependency is optional or mandatory), it's a good idea to only require it inside eval-when-compile, so you speed up the startup time once the package is already compiled.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

Fuzzy matching is a nom in Vim world, with various solutions.

Please explain, maybe link to some Vim sources. I've had a great kick at emulating Vim easymotion.

Have you tried to make Ivy work with my sample test of 250k files? If not, then Ivy is still not that fast.

It's a matter of approach. counsel-git-grep works fine for the Linux source with 19M lines, counsel-git is also very fast on the Linux source with 50k files.

Ivy does not have action list that allows multipole actions on a single candidate.

It does. counsel-describe-variable and counsel-describe-function have 3 actions:

  • RET describes.
  • C-. goes to definition.
  • C-, goes to Info description.
helm-colors: help me find a face and customize it quickly.

I'll put this one on the list.

Various helm-info-*commands: it's a joy to use this to quickly search for information.

A plain g Info-goto-node works great with ivy-mode.

helm-locate-library: help me quickly jump to a loaded Elisp file.

counsel-load-library is better: it has uniquify-style name resolution. Helm just gives you e.g.:

magit.el
magit.el

while Ivy gives:

magit/magit
site-lisp/magit
helm-locate: why should I need to browse to root directory to open any file? Just use the command and fuzzy match it, then open right here. No browsing needed.

This one is pretty cool. I'll add it to the list.

helm-pp-bookmarks

Ivy is better here with ivy-switch-buffer command, when ivy-use-virtual-buffers is enabled.

helm-recentf

Again, ivy-switch-buffer does this. But there's also ivy-recentf, since one user wanted a separate command.

helm-descbinds

Very nice, I may add a similar command. But C-h m + swiper can work here as well.

helm-ag

Seems much worse than even helm-git-grep. How is this even good?

helm-pt

Haven't heard of platinum before. How is it better than ag?

helm-gtags

I'm just using CEDET. I tried GTAGS ages ago, it was too simplistic for C++.

helm-themes

This is just the built-in load-theme with helm completion. If you enable helm-mode or ivy-mode and call load-theme, you'll get the same.

The ability to combine multiple sources.

This is developer-side. And as a developer that wrote a very sizable amount of Helm-enabled packages (I was very enthusiastic about it around a year ago), I can tell you that Helm is super-painful to debug, and very hard to completely understand. You can play games with it, copy-paste stuff, it still won't work exactly how you want if you want something more complex than completing-read. Just look at the implementation of the Helm back end of lispy-occur, or swiper-helm: it's super ugly, I'm surprised it works.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

As a general thing, if you only use macros from a package (regardless of whether the dependency is optional or mandatory), it's a good idea to only require it inside eval-when-compile, so you speed up the startup time once the package is already compiled.

Thanks. Question: how do I proceed from this code?

(eval-when-compile
  (if (featurep 'hydra)
      (progn
        (require 'hydra)
        (defhydra lh-knight ()
          "knight"
          ("j" lispy-knight-down)
          ("k" lispy-knight-up)
          ("z" nil)))
    (defun lh-knight/body ()
      (interactive)
      (error "Please install `hydra' package"))))

Suppose the user didn't have hydra installed when he installed (and byte-compiled lispy). Now he calls lh-knight/body to find the functionality is missing. But even if he installs hydra, lh-knight/body won't work since lispy isn't re-compiled. How could I solve this problem?

@tuhdo
Copy link
Author

tuhdo commented Jun 10, 2015

Please explain, maybe link to some Vim sources. I've had a great kick at emulating Vim easymotion.

If you join /r/vim and observe it daily, you will see. For example, this is yet another fuzzy matcher for Ctrl-P, a popular plugin in Vim. In the link, you will see it compares itself with several other matches.

Another popular plugin is Command-T.

Another popular plugin which is closet to Helm (but still has much less features) is Unite.

When a Vim user joins Spacemacs, they often ask about Ctrl-P equivalent in Emacs. Only Helm satisfies the requirement.

It does. counsel-describe-variable and counsel-describe-function have 3 actions:

Ok good to know. But Helm has a lot of useful actions, for example open file as root, open file other window, insert file path... Only Helm Projectile can do something like this at the moment.

A plain g Info-goto-node works great with ivy-mode.

But here you query entire index of an info manual, not just a page. Man page users don't like Info since they cannot use Vim to search within a page. But with Helm, such problem does not exist.

counsel-load-library is better: it has uniquify-style name resolution. Helm just gives you e.g.:

You can toggle the basename with C-]. I never see two duplicate file names anyway loaded in my Emacs anyway, so it makes sense to hide the path by default.

Ivy is better here with ivy-switch-buffer command, when ivy-use-virtual-buffers is enabled.

How does it compare with helm-mini? In helm-mini, I can narrow to buffers based on major modes. I can hide/show buffer paths with C-]. Each buffer has its own colour to identify its group i.e. I can easily see which file is a executable. helm-mini also sorts buffers according to history.

I'm just using CEDET. I tried GTAGS ages ago, it was too simplistic for C++.

I do not use gtags for code completion. I use it to navigate around and it works nicely. Fast to generate tag database and fast to jump. It also gives references, also there are wrong ones but it's ok. I can narrow easily with Helm.

This is just the built-in load-theme with helm completion. If you enable helm-mode or ivy-mode and call load-theme, you'll get the same.

With helm-theme, I can use C-<up> and C-<down> to cycle through themes to quickly try it out. With load-theme, it keeps asking me whether a theme is safe.

Seems much worse than even helm-git-grep. How is this even good?

It does not depend on git. It uses ag, so it's very fast. I love seeing instant result for whenever I search with helm-ag, in Linux source tree.

This is developer-side. And as a developer that wrote a very sizable amount of Helm-enabled packages (I was very enthusiastic about it around a year ago), I can tell you that Helm is super-painful to debug, and very hard to completely understand.

Maybe you should use the user APIs such as helm-build-sync/async-source, helm-make-actions. It will make code much simpler and always correct.

@Malabarba
Copy link

You can evaluate a macro at runtime like this:

(eval '(defhydra lh-knight ()
         "knight"
         ("j" lispy-knight-down)
         ("k" lispy-knight-up)
         ("z" nil)))

So you can wrap that in a (defun lispy--setup-hydra ...) and call that after hydra has been installed.

It's not pretty, but it's the only way to use a macro that wasn't available at compile time. Optional dependencies really aren't Emacs forte.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

If you join /r/vim and observe it daily, you will see. For example, this is yet another fuzzy matcher for Ctrl-P, a popular plugin in Vim. In the link, you will see it compares itself with several other matches.

I'm not that interested in Vim. But thanks for the links, I'm looking at Command-T now.

A plain g Info-goto-node works great with ivy-mode.

But here you query entire index of an info manual, not just a page.

Ah, I see. So it's topics+index, not just topics.

You can toggle the basename with C-].

That's a pretty good solution.

Ivy is better here with ivy-switch-buffer command, when ivy-use-virtual-buffers is enabled.

How does it compare with helm-mini?

It's invisible to the point that you don't notice it's there, and I think that's a great feature. Other than that, the candidates are there, but there's no stuff like mode narrowing.

With helm-theme, I can use C- and C- to cycle through themes to quickly try it out. With load-theme, it keeps asking me whether a theme is safe.

I see. Ivy binds similar things to C-M-n and C-M-p by default. The problem this can't be used within the completing-read-function interface by either Helm or Ivy, since all calls to completing-read-function don't provide the caller.

@Malabarba, do you think there's a chance to convince emacs-devel to extend the completing-read-function interface to supply the caller? It would allow each completion engine to have the "execute-action-but-not-exit-completion" command, without having to implement an overriding function in each case. For example, helm-themes would be the same as load-theme with helm-mode on, if the interface was extended.

Maybe you should use the user APIs such as helm-build-sync/async-source, helm-make-actions. It will make code much simpler and always correct.

Trust me, it's hard. helm-build-sync/async-source, helm-make-actions are extensions completing-read, which is a very simple interface. To actually make helm-swiper work, I had to use:

  • helm-display-function
  • helm-move-selection-after-hook
  • helm-update-hook
  • helm-after-update-hook
  • match-strict
  • filtered-candidate-transformer

All of these without an option to debug, just editing the code and hoping it would work.

@Malabarba
Copy link

@Malabarba, do you think there's a chance to convince emacs-devel to extend the completing-read-function interface to supply the caller? It would allow each completion engine to have the "execute-action-but-not-exit-completion" command, without having to implement an overriding function in each case. For example, helm-themes would be the same as load-theme with helm-mode on, if the interface was extended.

The completion engine could always just check the value of this-command.
Given that 95% of completing-read uses are inside an (interactive) clause, this variable should be reliable.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

The completion engine could always just check the value of this-command.
Given that 95% of completing-read uses are inside an (interactive) clause, this variable should be reliable.

It's not that simple: examining this-command means that each command has to be customized for. If completing-read-function took an optional ACTION argument, that takes one string it would work universally, 100%.

For example, I'm getting ~600 mentions of (switch-to-buffer in Emacs source. Obviously not interactive calls.

Here's what I'm talking about:

(defun switch-to-buffer-new (buffer-or-name &optional norecord force-same-window)
  (interactive)
  (completing-read "Switch to buffer: "
                   ;; ...
                   (lambda (buffer)
                     (switch-to-buffer buffer norecord force-same-window))))

When there's clearly one exit point after the completion takes place, it's possible to just let completion engine know how to finish the call and exit the function.

Advantage:

  • all completion engines get an execute-persistent-action command for free
  • all completion engines get a next-candidate-and-execute-persistent-action command for free
  • all completion engines get a resume-last-completion command for free

Disadvantage:

  • the calls to completion need to be updated, at their own pace.

@Malabarba
Copy link

It's not that simple: examining this-command means that each command has to be customized for. If completing-read-function took an optional ACTION argument, that takes one string it would work universally, 100%.

Yes, you're right.

Here's what I'm talking about: [...]

That suggestion looks plausible, and adding an extra argument to completing-read should be safe, so maybe you can get emacs-devel on board with this.
However, passing an extra argument to completing-read-function is not backwards compatible, so it's probably not a good idea.

The good thing is that you don't really need to pass the ACTION to completing-read-function. As long as completing-read knows what the action is, the completion engine can simply return a value like (no-exit . "completion-chosen"), and completing-read can take care of invoking the action and then re-invoking completing-read-function.

abo-abo added a commit that referenced this issue Jun 10, 2015
* lispy-pkg.el (hydra): Remove dependency.
(multiple-cursors): Remove dependency.

* lispy.el (defhydra): Add a stub macro that offers to install hydra and
  recompile/reload lispy when a hydra command is called.

Re #74
Fixes #46
@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

However, passing an extra argument to completing-read-function is
not backwards compatible, so it's probably not a good idea.

You're right. Maybe help-function-arglist could work here.

The good thing is that you don't really need to pass the ACTION to
completing-read-function. As long as completing-read knows what the
action is, the completion engine can simply return a value like
(no-exit . "completion-chosen"), and completing-read can take care
of invoking the action and then re-invoking
completing-read-function.

I think it's necessary: C-M-n in Ivy or C-down in Helm don't return to their caller. They call their lambda action and ask for more input.

With (no-exit . "completion-chosen") it could work, but then it would be completing-read -> completing-read-function -> completing-read -> completing-read-function. Looks ugly.

And stuff like helm-resume/ivy-resume wouldn't work without storing the lambda action, since these can be called long after completing-read has exited.

@Malabarba
Copy link

I think it's necessary: C-M-n in Ivy or C-down in Helm don't return to their caller. They call their lambda action and ask for more input.
With (no-exit . "completion-chosen") it could work, but then it would be completing-read -> completing-read-function -> completing-read -> completing-read-function. Looks ugly.

Currently, completing-read is literally this:

(funcall completing-read-function args...)

The alternative I'm suggeting is this:

(let ((completion (funcall completing-read-function args...)))
  (while (eq (car-safe completion) 'no-exit)
    (setq completion (funcall completing-read-function args...)))
  completion)

Is that too ugly?

And stuff like helm-resume/ivy-resume wouldn't work without storing the lambda action, since these can be called long after completing-read has exited.

Unless the pause/resume functionality is implemented directly in completing-read.

The advantage of letting completing-read handle all this action-related functionality is that you get less code redundancy between all those completion packages. The disadvantage is that you get less versatility.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

Is that too ugly?

It's a bit ugly, since I'd have to toggle between two functions when debugging.

Unless the pause/resume functionality is implemented directly in completing-read.

That's limiting. Here's another idea that I haven't implemented yet: ivy-persist - generate a buffer similar to occur. Clicking or pressing RET on each line calls ACTION for the current candidate. You could have as many persist buffers as you like, for as long as you want. Having flexibility means ideas like this are easier to implement.

The advantage of letting completing-read handle all this
action-related functionality is that you get less code redundancy
between all those completion packages. The disadvantage is that you
get less versatility.

True, but I'll take versatility over redundancy any time. That's what Emacs is about for me.

@syl20bnr
Copy link

Most important thing for dependencies is that they are actively maintained which is the case here. The fact that the author is the same for all dependencies has some values too. My concern is not on the size of lispy, @abo-abo mentioned the helm case with thousands of line of codes.

My concern is consistency, in Spacemacs we made the choice to use helm everywhere and I even ditched ido from it as a default for some tasks I prefer to do in ido (smex and Ido-find-file for instance). We also have a custom macro for transient maps. When a package is not modularized with isolated dependencies in package extensions it makes its integration harder which is sad when the package has great values.

I think that those improvements/extensions upon lispy should go in separated packages like it is often the case with helm or flycheck. It is beneficial on a architectural point of view but also on a practical point of view.

The good solution of using lispy combined with hydra and other packages should be something done in your distributed .emacs.d. Now I understand that this better architecture does not give you anything new so it's hard to ask you something that's purely for us.

I'm on a mobile excuse the typos ;-)

@Malabarba
Copy link

@abo-abo

That's limiting. Here's another idea that I haven't implemented yet: ivy-persist - generate a buffer similar to occur. Clicking or pressing RET on each line calls ACTION for the current candidate. You could have as many persist buffers as you like, for as long as you want. Having flexibility means ideas like this are easier to implement.

FWIW, completing-read could do that too. In that situation, the completion engine could return something like (occur . (“item1” “item2” ...)).

I admit I have a preference towards building this stuff into completing-read, but that's just because I'd really like to see these features in Emacs core. :)
But I'm not gonna be the guy to do that, and if your preference is to pass the ACTION argument to the function in completing-read-function than I'm in favor of that too (as long as it's careful to check the arity of the function).

As for the initial question. I think you can get the approval (or at the least, the non-disaproval) of the dev list on this. Here are some things I try to do when proposing important changes (not that you need it)

  • Keep it short
  • Be clear that it won't break anything.
  • Phrase it as a question not as a suggestion.

@abo-abo
Copy link
Owner

abo-abo commented Jun 10, 2015

Thanks for the help.

I'd really like to see these features in Emacs core. :)

Maybe Ivy will displace/join icomplete.el at some point. I've actually adapted it from icomplete initially.
I thought icomplete was relatively new, but turns out it was imported by RMS 22 years ago:)

Phrase it as a question not as a suggestion.

Nice strategy, I've never thought of that. I'll make sure to give it a try sometime:)

sooheon pushed a commit to sooheon/lispy that referenced this issue Sep 10, 2015
ivy.el (ivy-partial-or-done): Update.

Fixes abo-abo#74
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

No branches or pull requests

4 participants