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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Port the cljs-tooling completion machinery to compliment #62

Merged
merged 2 commits into from
Aug 11, 2019
Merged

Port the cljs-tooling completion machinery to compliment #62

merged 2 commits into from
Aug 11, 2019

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Mar 5, 2019 •
edited

This PR shows what CLJS completions look like.

The code is ported from cljs-tooling - what is used by Cider for ClojureScript completions.

The goal here is to end up with the same public api and behavior. Probably some
of the Compliment options would not apply in which case, please, advice 馃槃

I am always open to tweaks of any sort, just send any feedback over.

@Olical
Copy link
Contributor

Olical commented Mar 6, 2019

I'm extremely excited about this and will be hooking it into my prepl neovim tooling immediately. If I get around to it quickly enough I'll try to provide feedback and improvements where applicable.

@arichiardi
Copy link
Contributor Author

arichiardi commented Mar 6, 2019 •
edited

@Olical thank you! Note that this isn't working at the moment, I didn't have time yet to port the most important piece 馃槈

@arichiardi arichiardi force-pushed the port-cljs-tooling-completion branch 3 times, most recently from 6283469 to 097f17a Compare April 12, 2019 00:31
@arichiardi arichiardi mentioned this pull request Apr 12, 2019
Closed
@arichiardi arichiardi mentioned this pull request Apr 16, 2019
Closed
@arichiardi
Copy link
Contributor Author

Waiting for this on some feedback on #63 - the solution I have with dynamic vars is not really ideal imho

alexander-yakushev
alexander-yakushev reviewed Apr 17, 2019
View reviewed changes
project.clj Outdated Show resolved Hide resolved
project.clj Outdated Show resolved Hide resolved
project.clj Outdated Show resolved Hide resolved
src/compliment/sources/cljs.cljc Outdated Show resolved Hide resolved
src/compliment/utils.clj Outdated Show resolved Hide resolved
@arichiardi
Copy link
Contributor Author

Ok pushed the version with the dynamic var.

I have also a question: given that Clojure and ClojureScript completions are mutually exclusive (for instance I get or in both cljs.core and clojure.core) I was wondering if there is a way to include only some completion sources and not others...

Or maybe cider should configure that from the outside

@arichiardi arichiardi force-pushed the port-cljs-tooling-completion branch 6 times, most recently from 99859e9 to 245f7ba Compare April 20, 2019 18:09
@arichiardi
Copy link
Contributor Author

arichiardi commented Apr 22, 2019 •
edited

@alexander-yakushev during Clojure/North I got to a mostly usable version of the port.

I am missing still the :doc implementation but it is going to be very simple cause the compiler state basically contains it for most of the candidates.
I also probably don't strictly need it because Cider uses orchard for this.

In any case, maybe a second run of review would be good. Please remember that this is only 30% my code. Now I understand it completely, but the cljs-tooling code base was not really as optimized as this one.

I am hoping others and myself to give it more love over time when it's in compliment.

@arichiardi arichiardi force-pushed the port-cljs-tooling-completion branch 2 times, most recently from cb64a3d to f612191 Compare April 23, 2019 22:38
@arichiardi arichiardi force-pushed the port-cljs-tooling-completion branch from f612191 to 047eda2 Compare May 5, 2019 20:29
@arichiardi
Copy link
Contributor Author

Added the doc hook to the ClojureScript namespace today. The only TODO there is to return docstrings for macros. Small task.

A review now would be awesome.

@arichiardi arichiardi force-pushed the port-cljs-tooling-completion branch from 047eda2 to ad167dc Compare May 7, 2019 03:40
@arichiardi arichiardi changed the title WIP - Port the cljs-tooling completion machinery to compliment Port the cljs-tooling completion machinery to compliment May 7, 2019
@arichiardi
Copy link
Contributor Author

pingy-ping 馃敂

@alexander-yakushev
Copy link
Owner

I've skimmed through it. It looks pretty good!

I have also a question: given that Clojure and ClojureScript completions are mutually exclusive (for instance I get or in both cljs.core and clojure.core) I was wondering if there is a way to include only some completion sources and not others...

I've needed this before as well but there is yet no way to do it. Is it necessary for this PR? If not, I might be able to think about it again later and maybe come up with something.

@alexander-yakushev
Copy link
Owner

How the tests should be run with these changes? When I plainly run lein test on your branch, it fails with Caused by: java.io.FileNotFoundException: Could not locate cljs/env__init.class, cljs/env.clj or cljs/env.cljc on classpath.

@arichiardi
Copy link
Contributor Author

Yes you need to add the profile (in cider they use a Makefile for this).

Try:

 lein with-profile +1.10 test

@alexander-yakushev
Copy link
Owner

Sorry, missed that. Yes, that works.

@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 22, 2019 •
edited

I was thinking really that if the goal is to have the exact same api (I looove fuzzy completions) it might be maybe a better idea to include the correct broken-down Cljs implementations inside the current ones.

Meaning, a conditional on the presence of the dynamic var cljs-env could determine if ClojureScript completions are required.

Either that or to use/share the pieces of code from the Clojure completions that do fuzzy matching in the ClojureScript namespace.

Does it make sense?

@bbatsov bbatsov mentioned this pull request Jun 26, 2019
Merged
alexander-yakushev
alexander-yakushev reviewed Aug 10, 2019
View reviewed changes
project.clj Outdated Show resolved Hide resolved
project.clj Outdated Show resolved Hide resolved
src/compliment/utils.clj Show resolved Hide resolved
@arichiardi
Copy link
Contributor Author

Comments addressed.

@arichiardi
Port the cljs-tooling completion machinery to compliment
f8996a0
@arichiardi
Use a different keyword for the special tag test
ae4d775 
It seems the word :deprecated has other candidates matching so this change is
necessary to keep the validity of the tests.
@alexander-yakushev
Copy link
Owner

Alright chums, let's do this!

@alexander-yakushev alexander-yakushev merged commit fceda86 into alexander-yakushev:master Aug 11, 2019
@Olical
Copy link
Contributor

Olical commented Aug 11, 2019

馃槺馃帀馃帀馃帀

I'll be trying this in Conjure ASAP!

@arichiardi arichiardi deleted the port-cljs-tooling-completion branch August 11, 2019 17:09
@arichiardi
Copy link
Contributor Author

Happy this is in now! But please let's keep in mind that I have a better (kinda breaking) improvement..

@arichiardi
Copy link
Contributor Author

@alexander-yakushev it would be awesome to release a beta or snapshot version so that I can try it out in cider-nrepl

@alexander-yakushev
Copy link
Owner

Already did, 0.4.0-SNAPSHOT.

@arichiardi
Copy link
Contributor Author

Great thank you!

@arichiardi arichiardi mentioned this pull request Aug 15, 2019
Closed
@smolck smolck mentioned this pull request Feb 15, 2020
Closed
@alexander-yakushev alexander-yakushev mentioned this pull request Jun 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-yakushev alexander-yakushev alexander-yakushev left review comments

@vemv vemv vemv left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@arichiardi @Olical @alexander-yakushev @vemv