Skip to content

add callback argument to lumo get-completions #81

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

hlolli
Copy link
Contributor

@hlolli hlolli commented Jun 2, 2017

lumo.repl/get-completions takes two arguments.
https://github.com/anmonteiro/lumo/blob/master/src/cljs/snapshot/lumo/repl.cljs#L1252
So I added map str function into the callback, now I'm getting completions suggestions from company-mode.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • [x ] The commits are consistent with our contribution guidelines
  • [x ] The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@hlolli
Copy link
Contributor Author

hlolli commented Jun 2, 2017

Yes, the completions work, but arglists aren't appearing while scrolling the completion list via company. Could you test this and the other pull request I made, or look over it. I see that on lumo side, sometimes lumo gives wrong completions, there is to say, all possible completions, in my case after ) character.

Copy link
Contributor

@arichiardi arichiardi left a comment

Choose a reason for hiding this comment

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

I think it would be more clear and maybe correct to use a prn or (print (pr-str ...)) here just to make clear that this is a callback that writes out. I was even wondering why it is working now..We pass an anonymous callback that returns a list...however the return of get-completions is nil. Is it? I am confused 😀

@arichiardi
Copy link
Contributor

So the callback thing is a pain (notice the nil at the end because of the print) :

cljs.user=> (lumo.repl/get-completions "redu" #(print (map str %)))
(reduce reduce-kv reduceable? reduced reduced? reductions)nil

@hlolli
Copy link
Contributor Author

hlolli commented Jun 8, 2017

That nil should go trough the elisp read function and become ignored when it hit the case statement (because it's not in quotes and therefore treated as elisp nil). I regret making two pull requests as I see this are two different problems interleaved with one another.

@arichiardi
Copy link
Contributor

arichiardi commented Jun 8, 2017

I'd solve it this way:

(let [hack (atom)]
  (lumo.repl/get-completions "redu" #(reset! hack (map str %)))
  @hack)
cljs.user=> (let [hack (atom)] (lumo.repl/get-completions "redu" #(reset! hack (map str %))) @hack)
("reduce" "reduce-kv" "reduceable?" "reduced" "reduced?" "reductions")

@hlolli
Copy link
Contributor Author

hlolli commented Jun 8, 2017

Very good! Agree, I update the pull request and use your snippet 😄 Besides the nested % sign, doesn't want to quote it against emacs format function. Use fn [] instead.

@arichiardi
Copy link
Contributor

Fixed.

@hlolli
Copy link
Contributor Author

hlolli commented Jun 8, 2017

Are you commenting on the correct pull requests? This fixes it or you've fixed it?

@arichiardi
Copy link
Contributor

No sorry I have fixed the comment with the snippet, I cannot fix the PR 😄

@arichiardi
Copy link
Contributor

arichiardi commented Jun 8, 2017

This has another problem due to pretty printing, I will raise it in lumo.

@hlolli
Copy link
Contributor Author

hlolli commented Jun 8, 2017

PR updated.

@arichiardi
Copy link
Contributor

arichiardi commented Jun 8, 2017

As per the linked lumo issue, this avoid pretty pretting:

cljs.user=> (do (set! lumo.repl/*pprint-results* false) (let [hack (atom)] (lumo.repl/get-completions "redu" #(reset! hack (map str %))) @hack))
("Reduced" "reduce" "reduce-kv" "reduceable?" "reduced" "reduced?" "reductions")

Don't know if it matters actually, I haven't tried in inf-clojure, yet.

@@ -726,7 +726,10 @@ If you are using REPL types, it will pickup the most approapriate
(define-obsolete-variable-alias 'inf-clojure-completion-command 'inf-clojure-completion-form "2.0.0")

(defcustom inf-clojure-completion-form-lumo
"(doall (map str (lumo.repl/get-completions \"%s\")))"
"(let [ret (atom)]
Copy link
Contributor

@arichiardi arichiardi Jun 8, 2017

Choose a reason for hiding this comment

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

swap! accepts a function, reset! is better.

If you set it too:

(do (set! lumo.repl/*pprint-results* false) (let [hack (atom)] (lumo.repl/get-completions "redu" #(reset! hack (map str %))) @hack))

and squash all commits into one, we are good 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that and squash. But isn't this a bit mean to someone who wants to prettyprint the results when emacs is always setting it to false?

@arichiardi
Copy link
Contributor

Uhm, Antonio suggested it but you are right, it is global...Maybe this is the moment when we introduce a post processing step to completion. First I would try if it works with multiline lists, if not we have a fn that makes everything single line but we need to introduce some glue for post processing..

@hlolli
Copy link
Contributor Author

hlolli commented Jun 12, 2017

There are functions that can do that, but as far as I see, it returns a list of strings which is read as emacs data, so newline should be irrelevant, in my company-mode completions, makes no difference.

@hlolli hlolli force-pushed the lumo-completion branch from adf55d4 to 210f2e8 Compare June 12, 2017 12:54
@hlolli
Copy link
Contributor Author

hlolli commented Jun 12, 2017

Ok I changed swap! to reset! it works and gives completions, and also squashed the commits.

Little related @arichiardi , there's a bug in lumo
lumo_infclojure
This can be reproduced in lumo like this

(let [ret (atom)] (lumo.repl/get-completions "red)" (fn [res] (reset! ret (map str res)))) @ret)

Otherwise I think @bbatsov can merge this now.

@arichiardi
Copy link
Contributor

LGTM

@arichiardi arichiardi mentioned this pull request Jun 14, 2017
4 tasks
@bbatsov bbatsov merged commit 74e8423 into clojure-emacs:master Jun 20, 2017
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants