Skip to content

add expression break chars #110

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

Merged
merged 1 commit into from
Oct 5, 2017
Merged

add expression break chars #110

merged 1 commit into from
Oct 5, 2017

Conversation

hlolli
Copy link
Contributor

@hlolli hlolli commented Sep 2, 2017

To not get weird completions the expression boundries cant contain open or close parenthesis and open or close brackets. Otherwise, in the case of lumo, the completions will look like the following picture.

https://user-images.githubusercontent.com/6074754/27034766-3de33bbe-4f7f-11e7-821f-62204cd9f02d.png

So far after adding these chars I've not experienced this again.


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!

@arichiardi
Copy link
Contributor

Uhm I have never noticed that. I will confirm.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 2, 2017

I also got this, in Clojure symbols can never start with a number, so we need to find a way to mark sensible boundies. I got this in my console after the cursor was station next to the number 1000.

("1000*"
 "1000*1"
 "1000*2"
 "1000*3"
 "1000*assert*"
 "1000*clojurescript-version*"
 "1000*command-line-args*"
 "1000*e"
 "1000*flush-on-newline*"
 "1000*loaded-libs*"
 "1000*main-cli-fn*"
 "1000*ns*"
 "1000*out*"
 "1000*print-dup*"
 "1000*print-err-fn*"
 "1000*print-fn*"
 "1000*print-fn-bodies*"
 "1000*print-length*"
 "1000*print-level*"
 "1000*print-meta*"
 "1000*print-namespace-maps*"
 "1000*print-newline*"
 "1000*print-readably*"
 "1000*target*"
 "1000*unchecked-arrays*"
 "1000*unchecked-if*"
 "1000*warn-on-infer*"
 "1000+"
 "1000-"
 "1000->"
 "1000->>"
 "1000->ArrayChunk"
....etc....

@arichiardi
Copy link
Contributor

Also are we sure it is inf-clojure? Looks like a lumo issue.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 2, 2017

Well, both inf-clojure and lumo are guilty here. But inf-clojure shouldn'r be asking about completion for symbol starting with parenthesis or number, and lumo shouldn't be giving the result. I think less communication between lumo and inf-clojure is always a win, I'd say this is priority of the two guilty parties.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 2, 2017

I wonder if this statement on line 1227 is referncing here to the syntax table for symbol and word consistuents

(not (memq (char-syntax (following-char)) '(?w ?_)))

Try seeing if keeping only symbol consistuent fixes it, but I must say I've never encountered memq in elisp before.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 2, 2017

No that that memq statmenet had nothing to do with it, I fixed it by just adding all the number to this string of chars

(defconst inf-clojure-clojure-expr-break-chars " \t\n\"\'`><,;|&{()[]1234567890")

but now this looks bit ugly, but works and I don't get these bugs anymore. Maybe you can give this a try @arichiardi ? Try seeing if you start typing a number in a list (100) and wait for the completions to pop up, if you get the same, and if changeing inf-clojure-clojure-expr-break-chars repairs it?

@arichiardi
Copy link
Contributor

I will definitely give it a try, thanks for digging!

@arichiardi
Copy link
Contributor

I just wonder whether break chars influence something else, like font faces or paredit. Will try a couple of things.

@arichiardi
Copy link
Contributor

Does this solve both parenthesis and number problem? Will try it out today

@arichiardi
Copy link
Contributor

Ok I confirm that this patch fixes the boundary issue, and (red) does not trigger weird completions. It does not solve the number issue though.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 7, 2017

No I didn't add the numbers, wait few minutes...

@hlolli
Copy link
Contributor Author

hlolli commented Sep 7, 2017

Pull and try again the numbers

@arichiardi
Copy link
Contributor

It looks like it is working but now I am getting a call:

----CMD->
"(let [ret (atom nil)] (lumo.repl/get-completions \"ars\" (fn [res] (reset! ret (map str res)))) @ret)\n"

With this situation (where | is point):

cljs.user=> ("19999ars| ")

So it looks like it tries to complete ars, or whatever...not sure how to solve this, or if we need to solve it either. It is for sure better than before.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 7, 2017

Maybe the " char is causing the string to become a word boundry. I still don't understand this way of calculating a word boundry, all I know adding this makes it bit better. Maybe we wait for more comments. If you discover something, then please share. Thanks for looking at this!

@arichiardi
Copy link
Contributor

I agree with you, in any case this should be merged now because it improves things a lot, so good job! 👍

inf-clojure.el Outdated
@@ -1220,7 +1220,7 @@ See variable `inf-clojure-buffer'."
(inf-clojure--read-or-nil)
(inf-clojure--list-or-nil))))

(defconst inf-clojure-clojure-expr-break-chars " \t\n\"\'`><,;|&{(")
(defconst inf-clojure-clojure-expr-break-chars " \t\n\"\'`><,;|&{()[]1234567890")
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me the list of all digits should be replaced with something like [0-9] or [:digit]. Just check the exact syntax for this.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 25, 2017

I've updated the PR, understood a bit better that skip-chars-backward won't solve this, but rather looking at the string after the boundries have been estimated.

There's similar problem happening with getting arglists, that numbers are being looked for causing this

Assert failed: (symbol? sym)
	 Object.lumo.repl.resolve_var (NO_SOURCE_FILE <embedded>:6382:148)
	 lumo.repl.get_arglists (NO_SOURCE_FILE <embedded>:6516:140)
...etc..

I will fix that with another PR soon.

but with this PR I've effectively preventes inf-clojure from printing thousands of lines of completions when typing (1234 |)

@arichiardi
Copy link
Contributor

@bbatsov I have tested this and it works, just wanted to ack this and push a merge 😉

@bbatsov bbatsov merged commit b048358 into clojure-emacs:master Oct 5, 2017
@slipset
Copy link
Contributor

slipset commented Oct 26, 2017

It seems to me that this breaks Planck on my setup.
I'll investigate further tomorrow.
M-x set-variable RET inf-clojure-generic-cmd RET "planck"
Start inf-clojure and typing in something like (defn foo [] )RET does not evaluate.

@arichiardi
Copy link
Contributor

It would be awesome if you could enable the logging and see what is sent (if anything) to the repl

@slipset
Copy link
Contributor

slipset commented Oct 27, 2017

Some of it was actually my fault, see
#112

#113 added to the confusion :(

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.

4 participants