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

Integrate the new suitable ClojureScript completion #641

Merged

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Aug 29, 2019

This patch adds the new feature to cider-nrepl and also deprecates
cljs-tooling.

It depends on the new version of clj-suitable and compliment.

Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

Note: If you're just starting out to hack on cider-nrepl you might find
nREPL's documentation and the
"Design" section of the README extremely useful.*

Thanks!

@arichiardi arichiardi force-pushed the integrate-cljs-completions branch 3 times, most recently from bbb01f9 to f81ffd6 Compare January 20, 2020 21:37
@arichiardi arichiardi changed the title Integrate the new compliment ClojureScript functions Integrate the new suitable ClojureScript completion Jan 20, 2020
@arichiardi arichiardi changed the title Integrate the new suitable ClojureScript completion WIP - Integrate the new suitable ClojureScript completion Jan 20, 2020
@arichiardi arichiardi force-pushed the integrate-cljs-completions branch from f81ffd6 to 809df44 Compare January 21, 2020 00:09
@arichiardi arichiardi changed the title WIP - Integrate the new suitable ClojureScript completion Integrate the new suitable ClojureScript completion Jan 21, 2020
project.clj Outdated Show resolved Hide resolved
@arichiardi
Copy link
Contributor Author

Found a couple of problems, working on patches.

@arichiardi arichiardi force-pushed the integrate-cljs-completions branch 2 times, most recently from 0ffff4c to 98a6d61 Compare January 30, 2020 19:28
@bbatsov
Copy link
Member

bbatsov commented Feb 5, 2020

Roger that! FYI - I plan to cut a new release in about a week (for the IN/Clojure conference).

@arichiardi arichiardi force-pushed the integrate-cljs-completions branch 6 times, most recently from d80ef34 to be1cd01 Compare February 7, 2020 17:36
:compliment.sources.namespaces-and-classes/namespaces-and-classes
:compliment.sources.special-forms/special-forms])

(def +cljs-sources+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the + signs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a (questionable 😄) convention I got from boot for globals that are not ^:dynamic. I can get rid of it if we don't like it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird to me, plus it's not consistent with the rest of our code. Let's remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

:extra-metadata extra-metadata}))))
(binding [suitable-sources/*compiler-env* cljs-env]
(concat (complete/completions prefix (merge completion-opts {:sources +cljs-sources+}))
(when enhanced-cljs-completion? (suitable/complete-for-nrepl msg))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this just add one more compliment source?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far I understand, the enhanced completions are not a defsource at the moment, maybe @rksm can expand on that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's hear what he'll tell us about those. It just seems super strange to me to have one set of completions as compliment source and the others as something else.

@bbatsov
Copy link
Member

bbatsov commented Feb 7, 2020

You'll also have to to rebase and update the changelog.

@arichiardi arichiardi force-pushed the integrate-cljs-completions branch 4 times, most recently from 9769c93 to 4c6c131 Compare February 7, 2020 21:34
This patch adds the necessary setup for ClojureScript completions with
clj-suitable. It also inlines the call to cljs-complete so that we avoid having
a wrapper.
:dependencies [[nrepl "0.6.0"]
^:inline-dep [cider/orchard "0.5.5"]
^:inline-dep [thunknyc/profile "0.5.2"]
^:inline-dep [mvxcvi/puget "1.2.0"]
^:inline-dep [fipp "0.6.22"] ; can be removed in unresolved-tree mode
^:inline-dep [compliment "0.3.10"]
^:inline-dep [cljs-tooling "0.3.1"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone!

@bbatsov bbatsov merged commit 28b3192 into clojure-emacs:master Feb 8, 2020
@bbatsov
Copy link
Member

bbatsov commented Feb 8, 2020

I'll merge this as it is. We can fix the missing compliment source situation later.

@arichiardi arichiardi deleted the integrate-cljs-completions branch February 9, 2020 20:13
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

Successfully merging this pull request may close these issues.

2 participants