Improve the completion bounds detection (now with keywords) #129

arichiardi · Jan 22, 2018

It was missing, but it is actually used for identifying completions bounds.

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

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

arichiardi · Jan 22, 2018

This needs more work as other things are failing, like completing ^:private.

arichiardi · Jan 23, 2018

The two changes are running fine locally, the problem is the tests are failing for some reason.

arichiardi · Jan 23, 2018

I think I need some help with the tests because they are not very nice. For instance you cannot understand what is wrong when something fails.

bbatsov · Jan 23, 2018

Btw, you might want to also consider buttercup (https://github.com/jorgenschaefer/emacs-buttercup) for the tests. I find it much nicer to use than ert. I use it on a few projects and I like it quite a lot. I think the only thing ert has going is that it's built-in.

arichiardi · Jan 23, 2018

I have checked buttercup thoroughly and stamped upon jorgenschaefer/emacs-buttercup#37.

It seems things are in motion, slowly, but assess is probably the way for me to go.

It should end up in core and probably at some point will have BDD testing in it: https://github.com/phillord/assess/blob/master/README.md#roadmap

I so like the helper functions for buffer testing, which is basically what I am doing here, so if you don't mind I will try that first.

arichiardi · Jan 23, 2018

test/inf-clojure-tests.el

+                 (expect (ict-bounds-string (inf-clojure-completion-bounds-of-expr-at-point))
+                         :to-equal "deref"))))
+
+          (it "computes bounds for ^:keyword when point is after it"


This is failing still, but I moved to buttercup and assess

What is it actually matching?

it should compute :keyword without the hat

Now it is returning nil

And what happens when you try this interactively?

I tried not in a test, in normal buffer it was working

So if you open an inf-clojure-minor-mode buffer and put this in it:

^:pr|

In ielm you'll see:

ELISP> (inf-clojure-completion-bounds-of-expr-at-point) (2 . 5)

bbatsov · Jan 23, 2018

test/inf-clojure-tests.el

+              (ict-with-assess-buffers
+               ((a (progn
+                     (insert "@deref")
+                     (goto-char 7))))


Isn't your cursor right after the word when you do insert anyways?

Well, now I know 😉

arichiardi · Jan 26, 2018

I ended up improving the regex (big dragons in elisp regexes!) and adding keyword detection so that now tests are passing with flying green colors.

bbatsov · Jan 28, 2018

There's a conflict with your other PRs, but overall the changes look good.

They were both missing and therefore the inf-clojure was not able to identify completions bounds correctly.

For instance we want to avoid using substring-no-properties while typing ^| (where | is point) because it will error out.

The patch copies functionality from cider so that now inf-clojure can detect and extract keywords (colons included) when a symbol at point is not found.

This patch add ert test harness and some tests around the inf-clojure-completion-bounds-of-expr-at-point function.

arichiardi · Jan 29, 2018

@bbatsov Done, can't wait to have tests in 😄

bbatsov · Jan 30, 2018

Your wait is over. ;-)

arichiardi · Jan 30, 2018

Thank you @bbatsov!

arichiardi changed the title ~~Add @ (deref) to the expression breaking chars.~~ Add @ (deref) to the expression breaking chars. Jan 22, 2018

arichiardi force-pushed the add-deref-break-char branch from bc14f8a to a037ea1 Compare January 23, 2018 00:30

arichiardi force-pushed the add-deref-break-char branch from a037ea1 to 8d8ec41 Compare January 23, 2018 00:35

arichiardi changed the title ~~Add @ (deref) to the expression breaking chars.~~ Improve the completion bounds function Jan 23, 2018

arichiardi force-pushed the add-deref-break-char branch 2 times, most recently from edcca0b to 86fae72 Compare January 23, 2018 01:00

arichiardi force-pushed the add-deref-break-char branch from 86fae72 to 14fced8 Compare January 23, 2018 20:24

arichiardi commented Jan 23, 2018

View reviewed changes

bbatsov reviewed Jan 23, 2018

View reviewed changes

arichiardi force-pushed the add-deref-break-char branch from 14fced8 to 2feb03a Compare January 26, 2018 20:38

arichiardi changed the title ~~Improve the completion bounds function~~ Improve the completion bounds detection (now with keywords) Jan 26, 2018

arichiardi added 5 commits January 29, 2018 09:34

Add @ (deref) and ^ to the expression breaking chars

74a6fac

They were both missing and therefore the inf-clojure was not able to identify completions bounds correctly.

Avoid computing completion bounds when no valid chars are at point

213f6bd

For instance we want to avoid using substring-no-properties while typing ^| (where | is point) because it will error out.

Extract keyword words for completion

dbe2839

The patch copies functionality from cider so that now inf-clojure can detect and extract keywords (colons included) when a symbol at point is not found.

Add test harness (finally?)

ff63353

This patch add ert test harness and some tests around the inf-clojure-completion-bounds-of-expr-at-point function.

Update CHANGELOG.md

12ce769

arichiardi force-pushed the add-deref-break-char branch from f27cc86 to 12ce769 Compare January 29, 2018 17:34

bbatsov merged commit 630471b into clojure-emacs:master Jan 30, 2018

arichiardi deleted the add-deref-break-char branch January 30, 2018 03:00

Search code, repositories, users, issues, pull requests...

Uh oh!

Improve the completion bounds detection (now with keywords) #129

Improve the completion bounds detection (now with keywords) #129

Uh oh!

Conversation

arichiardi commented Jan 22, 2018

Uh oh!

arichiardi commented Jan 22, 2018

Uh oh!

arichiardi commented Jan 23, 2018

Uh oh!

arichiardi commented Jan 23, 2018

Uh oh!

bbatsov commented Jan 23, 2018

Uh oh!

arichiardi commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichiardi commented Jan 26, 2018

Uh oh!

bbatsov commented Jan 28, 2018

Uh oh!

arichiardi commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbatsov commented Jan 30, 2018

Uh oh!

arichiardi commented Jan 30, 2018

Uh oh!

Uh oh!

arichiardi commented Jan 29, 2018 •

edited

Loading