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

Add extra-metadata options to complete middleware #195

Merged
merged 3 commits into from
May 12, 2015

Conversation

Deraen
Copy link
Contributor

@Deraen Deraen commented Apr 23, 2015

Fixes #176

Merged: alexander-yakushev/compliment#26
Merged: gtrak/cljs-tooling/pull/18
Merged: gtrak/cljs-tooling/pull/19

@bbatsov In relation to alexander-yakushev/compliment#26

I decided to open this PR here for comments even though PR to Compliment is not merged yet and cljs-tooling doesn't have the support for options yet.

Added options which allow frontends to request arglists and doc for each completion results. Passes the options to Compliment.

@cichli
Copy link
Member

cichli commented Apr 26, 2015

This looks good to me, thanks for tackling the CLJS side of this as well!

@@ -57,7 +60,8 @@
:requires {"symbol" "The symbol to lookup"
"ns" "The symbol's namespace"
"session" "The current session"}
:optional {"context" "Completion context for compliment."}
:optional {"context" "Completion context for compliment."
"extra-metadata" "List of extra-metadata fields"}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should document the possible values extra-metadata can contain here too. AFAICT it's just :arglists and :doc for now?

@Deraen
Copy link
Contributor Author

Deraen commented Apr 28, 2015

Both PRs are merged.

@alexander-yakushev I think we need a new compliment release/snapshot?

@bbatsov
Copy link
Member

bbatsov commented Apr 28, 2015

@Deraen This could use a few specs.

@alexander-yakushev
Copy link
Member

And by specs he means tests.
(Automatic bbatsov translator 0.1.0)

On Tue, Apr 28, 2015, 08:39 Bozhidar Batsov notifications@github.com
wrote:

@Deraen https://github.com/Deraen This could use a few specs.


Reply to this email directly or view it on GitHub
#195 (comment)
.

@Deraen
Copy link
Contributor Author

Deraen commented Apr 28, 2015

Sure. I see there are tests for cljs complete I can extend but there seems to be no tests for clj complete yet?

I'll update the PR in the evening.

@bbatsov
Copy link
Member

bbatsov commented Apr 29, 2015

Sure. I see there are tests for cljs complete I can extend but there seems to be no tests for clj complete yet?

Hmm, thought we had some - if we don't guess you can use the cljs tests as a starting point.

@Deraen Deraen force-pushed the arglists-and-doc branch 3 times, most recently from 10fea7f to a0a50d4 Compare April 30, 2015 08:25
@bbatsov
Copy link
Member

bbatsov commented May 1, 2015

Some tests are failing, although the errors are pretty confusing to me. I've restarted the build. Btw, our conventions mandate the use of present simple tense for commit message titles.

@Deraen
Copy link
Contributor Author

Deraen commented May 1, 2015

Some tests are failing as cljs-tooling needs a fix to work with cljs-3196. And looks like some tests are failing because travis problems (Problem opening jar).

@alexander-yakushev
Copy link
Member

Seen that one before, restarting the tests worked for me.

On Fri, May 1, 2015, 09:43 Juho Teperi notifications@github.com wrote:

Some tests are failing as cljs-tooling needs a fix to work with cljs-3196.
And looks like some tests are failing because travis problems (Problem
opening jar).


Reply to this email directly or view it on GitHub
#195 (comment)
.

@expez
Copy link
Member

expez commented May 2, 2015

I've restarted this build twice now, but it keeps failing with the same problems.

@bbatsov
Copy link
Member

bbatsov commented May 4, 2015

@cichli You're our resident ClojureScript expert. What's your advise regarding cljs-3196?

@cichli
Copy link
Member

cichli commented May 12, 2015

Sorry for the delayed reply, totally missed this notification... the Travis caching problems are gone now. As @Deraen says, we just need gtrak/cljs-tooling#19 to be resolved and a new cljs-tooling release to be cut to fix the failing tests.

@Deraen
Copy link
Contributor Author

Deraen commented May 12, 2015

I reworded commit messages. Cljs-tooling change is hopefully now ok and is just waiting for @gtrak to check it.

The option allows frontends to request arglists and docstring metadata
for each completion results. The option is passed to Compliment
and Cljs-tooling.
@Deraen Deraen changed the title NOT READY: Added doc and arglists options to complete middleware Added doc and arglists options to complete middleware May 12, 2015
@Deraen
Copy link
Contributor Author

Deraen commented May 12, 2015

Updated with new cljs-tooling dep and everything should be working now.

@Deraen Deraen changed the title Added doc and arglists options to complete middleware Add doc and arglists options to complete middleware May 12, 2015
@Deraen Deraen changed the title Add doc and arglists options to complete middleware Add extra-metadata options to complete middleware May 12, 2015
bbatsov added a commit that referenced this pull request May 12, 2015
Add extra-metadata options to complete middleware
@bbatsov bbatsov merged commit d3dea40 into clojure-emacs:master May 12, 2015
@bbatsov
Copy link
Member

bbatsov commented May 12, 2015

👍

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 op to optionally return info for each var?
5 participants