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

Support company-mode? #30

Closed
dgutov opened this issue Apr 4, 2013 · 44 comments
Closed

Support company-mode? #30

dgutov opened this issue Apr 4, 2013 · 44 comments

Comments

@dgutov
Copy link

dgutov commented Apr 4, 2013

There is useful, non-ac-dependent code here that I'd rather not duplicate in a separate package. How should I go about using it in an nrepl completion back-end for company-mode, without forcing the users to install auto-complete (even automatically)?

I suggest removing the hard dependency, or extracting all non-ac-specific bits to another package (nrepl-mode?).

Pushing the Clojure code upstream might be an alternative, as suggested in #15.

@purcell
Copy link
Member

purcell commented Apr 4, 2013

Yeah, I've been thinking about this recently. I'd be happy to drop the auto-complete dependency, and to shuffle around the code to be more easily usable by both completion UIs.

@zendeavor
Copy link

i've just picked up clojure myself and am curious if this managed to get anywhere.

@purcell
Copy link
Member

purcell commented Sep 14, 2013

My favourite option would indeed be to push the clojure code upstream so that a company-nrepl would be trivial to write. And now that I'm a committer for nrepl (along with @bbatsov), that should probably be easier to do.

The bad news is that I'm super busy just now, and not doing much clojure, so the working set in my brain isn't ideal for getting this done without taking time to swap stuff back in. ;-)

@bbatsov
Copy link
Member

bbatsov commented Sep 14, 2013

@purcell Yep, that sounds like the best decision to me, too.

@dgutov
Copy link
Author

dgutov commented Sep 15, 2013

I can see that nrepl.el already has some completion support (nrepl-dispatch-complete-symbol).
Is it usable?

Together with nrepl-doc, it should be enough to create a half-decent backend for Company, at least the initial version.

@purcell
Copy link
Member

purcell commented Sep 16, 2013

There's a lot of clojure inline in ac-nrepl, because what was in nrepl was not adequate, so I need to review the current code in both libraries.

@dgutov
Copy link
Author

dgutov commented Sep 16, 2013

Hmm, what about Ritz middleware? nREPL delegates to its complete operation, when available, and it seems to provide the same search space as this package:

https://github.com/pallet/ritz/blob/develop/nrepl-middleware/src/ritz/nrepl/middleware/simple_complete.clj
https://github.com/pallet/ritz/blob/develop/repl-utils/src/ritz/repl_utils/completion.clj

@bbatsov
Copy link
Member

bbatsov commented Sep 16, 2013

If it were up to me nrepl.el would be relying on nREPL middleware only for completion, but the problem with middleware is that people have to install it :-) Generally I think it's not wise to embed a lot of foreign code into Elisp - it's messy, hard to comprehend and easy to get wrong.

@dgutov
Copy link
Author

dgutov commented Sep 16, 2013

Right, we'd have to document it, the users will need to pay attention, but as long as the middleware works, and as long as there's some fallback, which nrepl.el provides, I think it should be fine.

@purcell
Copy link
Member

purcell commented Sep 16, 2013

@bbatsov I agree: I've never liked having the clojure code embedded, not least because the long strings are ugly and error-prone. I even wrote an (unmerged) change which at least allowed me to convert elisp sexps into Clojure, e.g. https://github.com/clojure-emacs/ac-nrepl/blob/print-elisp-sexps-to-clojure/ac-nrepl.el#L110

@steckerhalter
Copy link

Is something happening here? I have started to use company as auto-complete really got on my nerves for being slow and buggy.

I started porting ac-nrepl to a company backend. Probably we could get together and create a new package that supports both backends? maybe named something like cider-complete?

btw, I'm not fond of having to add middleware to clj projects for emacs completion so I would prefer the native solution

@steckerhalter
Copy link

for what it's worth, here's my company backend: https://github.com/steckerhalter/company-cider

seems to work quite well already. now I can get back to what I really wanted to do :)

@dgutov
Copy link
Author

dgutov commented Jan 26, 2014

here's my company backend

Cool. :) Thanks for doing this.

@bbatsov
Copy link
Member

bbatsov commented Jan 26, 2014

Why can't cider's `cider-dispatch-complete-symbol' be used directly? After all - it returns a list of completions for a string.

Relying only on the presence of core.complete is not particularly wise, as a middleware providing a "complete" op might do it in a better way. For instance, cider's own "complete" middleware supports both Clojure & ClojureScript, which core.complete is Clojure-only.

@steckerhalter
Copy link

Why can't cider's `cider-dispatch-complete-symbol' be used directly? After all - it returns a list of completions for a string.

It is used: https://github.com/steckerhalter/company-cider/blob/master/company-cider.el#L81

Java methods are not currently covered by the function so I left that in from ac-nrepl.

@purcell
Copy link
Member

purcell commented Jan 26, 2014

This seems like an opportunity to push the java method completions up into cider, right? Then we can slim down both ac-nrepl and company-cider.

@bbatsov
Copy link
Member

bbatsov commented Jan 26, 2014

@purcell Shouldn't this code be in core.complete?

@purcell
Copy link
Member

purcell commented Jan 26, 2014

Probably yes in the longer term, and it's worth pursuing, but that path takes much longer than pushing this common code into a shared place in the short term.

@purcell
Copy link
Member

purcell commented Jan 26, 2014

We've tended to not rely on getting core.complete patched, because people have typically used ac-nrepl with every clojure version (and core.complete version) under the sun, so it would be wrong to assume that users will have a working version.

@dgutov
Copy link
Author

dgutov commented Jan 28, 2014

I think the code should be both moved to core.complete (how much is there to move anyway? at a cursory glance, the current clojure-complete provides more or less the same features), and pushed to cider or a separate package.

Some already existing methods in clojure-complete will just receive a face-lift, and the new Emacs package (say, nrepl-complete) would just use them, shifting the responsibility of using the latest version of clojure-complete to the users, who otherwise won't get as comprehensive or precise completions as they could (hopefully, without errors, though).

For any new methods, nrepl-complete could try calling them, and in case of failure use the "inline" fallback code that would be kept in there for compatibility.

Overall, I'd be more in favor of moving this to cider, it would just be a new sub-package, of which there are several already. It would also allow to upgrade the built-in cider-complete-at-point.

Or we could just add company-cider to MELPA and forget all this code-pushing business. ac-nrepl isn't seeing a lot of changes lately, so the value of having the code in one place is lower than it could be.

@steckerhalter
Copy link

How about including support for both ac and company directly into cider? Then cider could detect if one of the completion modes is active and enable the correct backend or do nothing (cider completion variable default -> auto).

@bbatsov
Copy link
Member

bbatsov commented Jan 28, 2014

I'm with @dgutov. It would be odd if cider-complete-at-point provided different completions than the alternative packages.

@steckerhalter Not sure about the auto-detection idea, since some people might have ac or company installed, but don't actually use them.

@purcell
Copy link
Member

purcell commented Jan 28, 2014

Yeah, cider shouldn't have to know anything about the completion back-ends.

@steckerhalter
Copy link

Not sure about the auto-detection idea, since some people might have ac or company installed, but don't actually use them.

I would just take the global modes as an indicator, so either global-auto-complete-mode or global-company-mode. Just having the package installed would not be considered.

But I'm not sure if this make sense if the providers are not included with cider.

@dgutov
Copy link
Author

dgutov commented Jan 28, 2014

A good indicator might be just whether company-backends or ac-sources are bound as a variable. If the user doesn't use the respective package, the change to a variable's value wouldn't hurt either.

But I think specialized completion backends should be kept separate. It would make sense, though, to sneak in Company support via the :company- properties into cider-complete-at-point, like lisp-completion-at-point does in the current Emacs trunk. Then company-cider would just provide a native backend, which is slightly faster in theory, for users of older Emacsen, and both company-cider and ac-nrepl could be slimmed down significantly by using those new property functions.

@steckerhalter
Copy link

@dgutov sounds like a good plan.

now what are we going to do with company-cider?

  1. merge into company
  2. push it to MELPA
  3. move it to clojure-emacs here and push it to MELPA

@dgutov
Copy link
Author

dgutov commented Jan 29, 2014

3 sounds best to me.

@bbatsov
Copy link
Member

bbatsov commented Jan 30, 2014

@steckerhalter 👍 for option 3 as well.

@bbatsov
Copy link
Member

bbatsov commented Jan 30, 2014

@steckerhalter @dgutov I've added you to the clojure-emacs organization, so you can transfer the repo at any point now. Let me know when this is done, so that I can grant you ownership for it within the organization.

@steckerhalter
Copy link

@bbatsov it is done

@bbatsov
Copy link
Member

bbatsov commented Jan 30, 2014

@steckerhalter I've given you and @dgutov ownership of the repo. When you've identified the code you want to promote upstream to cider - just fire up a PR.

@dgutov
Copy link
Author

dgutov commented Jan 30, 2014

Thanks. I'll do that once the immediate tasks I have planned for company-mode are done, and if no one beats me to it.

@gtrak
Copy link

gtrak commented Feb 18, 2014

the new cider-nrepl package takes care of many of these concerns. The intention is to remove any dependency on core.complete from cider.

Here's an impl of 'complete' that delegates to core.complete for clojure and my own package for clojurescript.

https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/complete.clj

@dgutov
Copy link
Author

dgutov commented Feb 19, 2014

Does info return var's arglist if it's a function? Otherwise, it seems like cider-nrepl provides enough operations to implement all required commands. And obtain better cljs support as a result, I suppose.

Using middleware sounds fine to me, but I don't do much Clojure, and some others expressed preference that all features should work without including middleware in each project, too.

@steckerhalter, have you tried including cider-nrepl globally via ~/.lein/profiles.clj? It might be the most hassle-free approach, on all sides.

@bbatsov
Copy link
Member

bbatsov commented Feb 19, 2014

@dgutov Middleware is the way to go. While I understand the desire of people to have an easier setup, the inlined Clojure code approach is not scalable (or portable) and the day will come when we'll have to drop most of it.

@steckerhalter
Copy link

have you tried including cider-nrepl globally via ~/.lein/profiles.clj? It might be the most hassle-free approach, on all sides.

@dgutov I just tried cider-nrepl once. currently it doesn't seem to add anything the default solution does not provide so I see no reason to use it at the moment.

@gtrak
Copy link

gtrak commented Feb 19, 2014

The middleware provides cljs support, there's no other way to do it, and
arg lists do come back in the info op. Also I've been fixing bugs as I've
found them, for instance jumping to protocol methods.

On Wednesday, February 19, 2014, steckerhalter notifications@github.com
wrote:

have you tried including cider-nrepl globally via ~/.lein/profiles.clj? It
might be the most hassle-free approach, on all sides.

@dgutov https://github.com/dgutov I just tried cider-nrepl once.
currently it doesn't seem to add anything the default solution does not
provide so I see no reason to use it at the moment.

Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-35496883
.

@dgutov
Copy link
Author

dgutov commented Feb 19, 2014

and arg lists do come back in the info op

Thanks!

@steckerhalter @bbatsov Like discussed, I'd like to push some reusable code to cider itself, but it seems that duplicating the middleware code in elisp there is not the way.

So I'm inclined to just implement advanced Company support in cider-complete-at-point using the middleware operations.

@gtrak
Copy link

gtrak commented Feb 19, 2014

It's going to be hard to maintain a ton of inlined eval code, breaking the
ice on server-nrepl code means we can start integrating with tooling
projects and sharing and borrowing ops with light-table and other nrepl
clients.

Cljs is a motivator, but the underlying issue is coupling cider to global
jvm state, which can be limiting in other ways.

Using cider-nrepl as a plugin in your user profile is hassle-free, and
hopefully once we add some more features, compelling too :-).

On Wednesday, February 19, 2014, Gary Trakhman gary.trakhman@gmail.com
wrote:

The middleware provides cljs support, there's no other way to do it, and
arg lists do come back in the info op. Also I've been fixing bugs as I've
found them, for instance jumping to protocol methods.

On Wednesday, February 19, 2014, steckerhalter <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

have you tried including cider-nrepl globally via ~/.lein/profiles.clj?
It might be the most hassle-free approach, on all sides.

@dgutov https://github.com/dgutov I just tried cider-nrepl once.
currently it doesn't seem to add anything the default solution does not
provide so I see no reason to use it at the moment.

Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-35496883
.

@gtrak
Copy link

gtrak commented Feb 19, 2014

Also, there is no hard requirement on cider-nrepl yet, I don't know if that means we have to duplicate code, at least it has to have sensible fallbacks.

I created an issue to track this:
clojure-emacs/cider#478

@dgutov
Copy link
Author

dgutov commented Feb 21, 2014

I don't know if that means we have to duplicate code, at least it has to have sensible fallbacks.

In the case of completion backend, we can just disable some non-essential functions.

@dgutov
Copy link
Author

dgutov commented Feb 28, 2014

@bbatsov I'd like to reuse cider-doc-handler and cider-jump-to-def, but the former and a part of the latter (cider--jump-to-def-eval-fn) are working asynchronously, which might've made sense from the perspective of working with nrepl, but AFAICS offers no tangible benefits.

Supporting asynchronous backend functions would require more work from a completion front-end (we have an issue for that: company-mode/company-mode#62). Would you mind if I make them work synchronously?

Or should I wait until company can handle async? For the above features (doc and jump-to-def) that would mean just moving the sleep-for logic to the backend caller function in company-mode.

@bbatsov
Copy link
Member

bbatsov commented Feb 28, 2014

@dgutov I think it's OK to make them synchronous. It seems to me that outside of evaluations in the main session, most tooling related ops are synchronous in nature anyways (like completing, displaying documentation, etc). I've changed some of those already, but the code is still pretty far from maturity.

@dgutov
Copy link
Author

dgutov commented Mar 3, 2014

clojure-emacs/cider#490 has been merged, so this can be closed now.

@steckerhalter Turns out, it doesn't require the cider-nrepl middleware either, at least in the current version. CIDER has enough inlined Clojure code of its own.

@dgutov dgutov closed this as completed Mar 3, 2014
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

6 participants