Skip to content

make case when nil is a string #80

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

Closed
wants to merge 2 commits into from
Closed

Conversation

hlolli
Copy link
Contributor

@hlolli hlolli commented Jun 2, 2017

I'm getting eldocs in lumo for symbol: nil, that's due to
line 1170:
(value (inf-clojure-eldoc-arglists thing))
returning "nil" as string so when is always getting truthy value.

Adding case of "nil" as string fixes this, but I guess this string "nil" is the return value from lumo.


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

Need to change this a bit, eldoc throws an error when a list type is passed trough this boolean, nothing strange about that. Except that eldoc is getting result from completions, something's wrong.

@hlolli
Copy link
Contributor Author

hlolli commented Jun 2, 2017

Ok, please look over if I'm not breaking any style-guide.

The problem was in nutshell that prn-str from get-arglists turn nil into a string on the lumo side, so I solved this differently, and trim to prevent multiline vectors from taking too many lines in the echo buffer.

Also, probably because of callback-hell of node-world, the completions were popping into the minibuffer, I solved this with the hammer that lists arriving from lumo aren't acceptable arglists/eldoc, that solves it. But then again, eldoc seems to slow down completions, as if emacs waits ~0.1 secs while typing. But that was like that before so I can look later into how to optimize.

I guess squashing can take place on github merge?

@arichiardi
Copy link
Contributor

arichiardi commented Jun 2, 2017 via email

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 don't like the fact that we are brancing out in two places here, one is the custom Lumo form and the other is in the handling code. The former should be probably generic. I wonder if planck has the same behavior here.

@hlolli
Copy link
Contributor Author

hlolli commented Jun 6, 2017

I'll give plack a try and see if this is also there.

It seemed to be the only way not to get the completions popping up in the minibuffer, it's an async problem with eldoc and subprocces in comnit. Eldoc needs to know how to differentiate between that data it receives, since completions are always a list, this solves it. Otherwise we can try figuring out how to avoid these race conditions.

@arichiardi
Copy link
Contributor

I think I need to see it, I was working on the other fix yesterday and they were eventually popping out normally. Maybe you can test #82 and tell me what happens?

@arichiardi
Copy link
Contributor

arichiardi commented Jun 15, 2017

I made a change that hopefully include this one as well in #82, and exactly here: https://github.com/clojure-emacs/inf-clojure/pull/82/files#diff-6264d22fc9d63a6499f1027f6330d982R991

Does this solve?

@@ -982,7 +983,9 @@ See variable `inf-clojure-arglists-form'."
(cond
((null arglists-data) nil)
((stringp arglists-data) arglists-data)
((listp arglists-data) arglists-result))))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember why I was returning the result here, it looks like I should return the read data...Uhm..

@arichiardi
Copy link
Contributor

arichiardi commented Jul 2, 2017

Pushed a new version that solves this (I hope) nicely: https://github.com/clojure-emacs/inf-clojure/pull/85/files#diff-6264d22fc9d63a6499f1027f6330d982R988

@bbatsov bbatsov closed this Jul 3, 2017
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.

3 participants