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

Introduce inf-clojure-completions-fn defcustom #122

Merged
merged 1 commit into from
Jan 2, 2018

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Dec 31, 2017

Now the user can provide a custom parsing function for completion and therefore
has complete (pun intended) freedom in what to use for it. Some could use
compliment completion for instance or even use directly what cider provides.
The defcustom defaults to inf-clojure-list-completions, which can only parse
candidates coming as a Lisp list of strings.


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
  • 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!

inf-clojure.el Outdated
"Return completions for the Clojure expression starting with EXPR.

Its only ability is to parse a list of candidate strings, every
other EXPR will be discarded and nil will be returned."
Copy link
Member

Choose a reason for hiding this comment

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

Isn't EXPR only the completion prefix? Probably you didn't mean to reference it here. On a related note - we might pick a better name for this param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will refactor

inf-clojure.el Outdated
with the candidates completion. See \\[completion-table-dynamic]
for more details.

The default value is the `inf-clojure-list-completions' function,
Copy link
Member

Choose a reason for hiding this comment

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

As the the default function does more than parsing the results (it's also invokes the Clojure completion code) this should probably the adjusted.

inf-clojure.el Outdated
(when (not (string-blank-p expr))
(thread-first
(format (inf-clojure-completion-form) (substring-no-properties expr))
(inf-clojure--process-response (inf-clojure-proc) "(" ")")
(inf-clojure--read-or-nil)
(inf-clojure--list-or-nil))))

(defcustom inf-clojure-completions 'inf-clojure-list-completions
Copy link
Member

Choose a reason for hiding this comment

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

Won't inf-clojure-completions-fn (or something like this) be a better name?

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 like this better yes

inf-clojure.el Outdated
@@ -1254,15 +1254,43 @@ See variable `inf-clojure-buffer'."
"Return DATA if and only if it is a list."
(when (listp data) data))

(defun inf-clojure-completions (expr)
"Return a list of completions for the Clojure expression starting with EXPR."
(defun inf-clojure-list-completions (expr)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this name, as this function also invokes the code which generates the list it eventually processes. Not sure we won't to decouple only the part that actually processes the results. The way I see it completion has 3 stages:

  • decide what's the Clojure code that will do the completion (e.g. we can set this per project or have some abilities check - try if compliment is present, fallback to core.complete if this fails. Probably the first option would be easier and a better initial approach to this. )
  • invoke that code and get some result from it
  • decide how to process the results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think I will take the send response out of the default function. I did not notice that inf-clojure-process-response was there. In my mind only the string should be passed down to the custom parsing function, as you said above.

@arichiardi arichiardi changed the title Make inf-clojure-completions a defcustom Introduce inf-clojure-completions-fn defcustom Jan 2, 2018
Now the user can provide a custom parsing function for completion and therefore
has complete (pun intended) freedom in what to use for it. Some could use
compliment completion for instance or even use directly what cider provides.
The defcustom defaults to inf-clojure-list-completions, which can only parse
candidates coming as a Lisp list of strings.
@bbatsov bbatsov merged commit c4adc89 into clojure-emacs:master Jan 2, 2018
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.

2 participants