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

Complete local bindings for ClojureScript files #767

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

iarenaza
Copy link
Contributor

When editing a ClojureScript file, cider-nrepl doesn't offer any local binding names (the function name and argument names if inside the function, the let-like block bindings if inside a let-like block, etc.) as completion candidates, like it does when editing a Clojure file.

The root of the problem is that
cider.nrepl.middleware.complete/cljs-sources only includes ::suitable-sources/cljs-source as a possible source. And suitable doesn't perform any local binding analysis.

Given that the local binding analysis done in
compliment.sources.local-bindings namespace doesn't perform any evaluation or execution of the context form (thus, it is independent of the actual host platform differences), we can use that same source for ClojureScript local bindings completion.

[Closes: #766]

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)

Thanks!

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

lgtm! Great find.

test/cljs/cider/nrepl/middleware/cljs_complete_test.clj Outdated Show resolved Hide resolved
@bbatsov
Copy link
Member

bbatsov commented Jan 31, 2023

Given that the local binding analysis done in
compliment.sources.local-bindings namespace doesn't perform any evaluation or execution of the context form (thus, it is independent of the actual host platform differences), we can use that same source for ClojureScript local bindings completion.

Good catch, I had missed this myself. It might be good to add some comment about this in the code, as at a glance it seems weird to mix ClojureScript and Clojure completion sources.

@iarenaza
Copy link
Contributor Author

It might be good to add some comment about this in the code, as at a glance it seems weird to mix ClojureScript and Clojure completion sources.

Do you mean in the definition of cider.nrepl.middleware.complete/cljs-sources or in the compliment.sources.local-bindings namespace? I guess it's the former, but just to make sure 😄

@bbatsov
Copy link
Member

bbatsov commented Jan 31, 2023

The former, although it probably won't hurt to expand the compliment source documentation at some point.

@@ -36,7 +36,8 @@

(def cljs-sources
"A list of ClojureScript completion sources for compliment."
[::suitable-sources/cljs-source])
[::suitable-sources/cljs-source
:compliment.sources.local-bindings/local-bindings])
Copy link
Member

Choose a reason for hiding this comment

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

Basically when I saw this line my first reaction was "how can this possibly work?". 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added (and PR updated)

@@ -2,6 +2,8 @@

## master (unreleased)

* [#766](https://github.com/clojure-emacs/cider-nrepl/issues/766) Complete local bindings for ClojureScript files.
Copy link
Member

Choose a reason for hiding this comment

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

This should be under "New features".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

When editing a ClojureScript file, cider-nrepl doesn't offer any local
binding names (the function name and argument names if inside the
function, the let-like block bindings if inside a let-like block,
etc.) as completion candidates, like it does when editing a Clojure
file.

The root of the problem is that
`cider.nrepl.middleware.complete/cljs-sources` only includes
`::suitable-sources/cljs-source` as a possible source. And `suitable`
doesn't perform any local binding analysis.

Given that the local binding analysis done in
`compliment.sources.local-bindings` namespace doesn't perform any
evaluation or execution of the context form (thus, it is independent
of the actual host platform differences), we can use that same source
for ClojureScript local bindings completion.

[Closes: clojure-emacs#766]
@bbatsov bbatsov merged commit 8899d93 into clojure-emacs:master Jan 31, 2023
@bbatsov
Copy link
Member

bbatsov commented Jan 31, 2023

Thanks!

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2023

@iarenaza I've cut a new release including your change (0.30) and I've updated CIDER's master to use it.

@iarenaza
Copy link
Contributor Author

iarenaza commented Feb 1, 2023

@iarenaza I've cut a new release including your change (0.30) and I've updated CIDER's master to use it.

Great!

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.

Complete local bindings for ClojureScript files
3 participants