Function arglists

[Deraen]

I don't know about Emacs but with Vim-fireplace I would very much like to display the arglist for each completion result.

This change enchances tag-fn to assoc arglists to the result if the var has one.

[bbatsov]

I don't quite get the rationale behind this. The arglist can (and should) be retrieved separately when needed IMO. That's what we do in Emacs and there's no problem to do so in vim.

[alexander-yakushev]

Doesn't vim-fireplace show a documentation popup for completion? That's where we show the arglist and other docs. Arglists are often too long and squeezing them into candidates can be cluttering.

[bbatsov]

Arglists are often too long and squeezing them into candidates can be cluttering.

Passing more data will also introduce an additional decoding overhead.

[Deraen]

Well, Vim doesn't really have a "documentation popup". Completion plugins, like YouCompleteMe, can display info for the selected candidate in a separate preview window.

But it looks like even the preview window can only display data retrieved from complete function. There is no separate API to retrieve the info when user changes the selection.

[alexander-yakushev]

That makes two of the frontends that want this behavior (see also clojure-emacs/cider-nrepl#176). It is possible to make this thing optional, so that tagged version returns arglists, docs or both. Although there might be some drop in performance.

Juho, are you one of the vim-fireplace maintainers? If not, whom should I cast into this thread for the discussion?

[bbatsov]

//cc @tpope

[bbatsov]

I have also some terrible memories of some arglists not being properly encodable in bencode, therefore we started sending them around as strings. Don't recall if we ever fixed this or not in CIDER and I don't have much time to investigate this.

There's one more thing to be considered - ideally changes like this should be synced with cljs-tooling.

[alexander-yakushev]

So the ElDoc middleware sends a string and CIDER parses it? vim-fireplace could do that too, I guess.

[bbatsov]

So the ElDoc middleware sends a string and CIDER parses it? vim-fireplace could do that too, I guess.

Actually the middleware returns a list of list of strings for each arglist if memory serves. Parsing this on the client was impossible at the time, as we were reading the Clojure code with Elisp's reader. Maybe @expez's edn.el can solve some of those problems.

P.S. You can examine the message exchange in *nrepl-messages*.

[bbatsov]

It is possible to make this thing optional, so that tagged version returns arglists, docs or both. Although there might be some drop in performance.

We can have some option in the middleware op to select just certain keys from the metadata.

[Deraen]

Btw. here are the changes for fireplace to support this: tpope/vim-fireplace@master...Deraen:master I'm thinking I'll open PR there when we get this solved.

I was testing this with fireplace and I don't see any performance problems even when completing with empty prefix, which results in around 800 suggestions.

Adding a option to cider-nrepl complete operation sounds like a good idea to me.

[alexander-yakushev]

Since I support Android as first-class platform, there are performance considerations there, so this behavior certainly won't become non-conditional. I would still like to know first if a more sensible solution, like ElDoc, is possible in Vim.

@bbatsov I would rather not rely on cider-nrepl to select the necessary keys from the result maps. There are projects that use Compliment directly (at least, Nightcode) and I don't want them to build additional middleware on top of completion. It is better to pass another option in options-map to Compliment and let it figure out what returns.

[bbatsov]

Fine by me.

[tpope]

Vim's completion provides room in the drop down menu next to each candidate for an additional annotation, which suits the arglist perfectly. This means I need all of them, not just the one for the selected function. Since it's for display, I would also greatly favor them in string form, as I don't have a Clojure printer available.

Limitations of the completion system mean that to automatically display docs, I would need them included as well. Personally this is not a high priority for me. I can tell you my eval solution includes arglists and docs and I've never hit on a performance problem.

[Deraen]

Would the following actions suit everyone:

1. I'll edit this patch to use additional options to enable attaching arglists and/or docstrings to the result
   • should they be two separate options or an option which takes in a set?
   • is everyone okay with passing the arglists as a string?
2. Similar options are added to cljs-tooling
3. Options are added to cider-nrepl
4. I'll send a PR to Vim-fireplace

[bbatsov]

is everyone okay with passing the arglists as a string?

Lists of strings.

The rest sounds good to me.

[Deraen]

Does the approach I took to pass the options map to tag-fn seem acceptable?

Example output:

user=> (compliment.core/completions "assoc" {:ns 'user, :tag-candidates true, :tag-candidates-doc true, :tag-candidates-arglists true})
({:arglists ("[map key val & kvs]" "[map key val]")
  :candidate "assoc"
  :doc "clojure.core/assoc\n([map key val] [map key val & kvs])\n  assoc[iate]. When applied to a map, returns a new map of the\n    same (hashed/sorted) type, that contains the mapping of key(s) to\n    val(s). When applied to a vector, returns a new vector that\n    contains val at index. Note - index must be <= (count vector).\n"
  :ns "clojure.core"
  :type :function}

[bbatsov]

I'd drop the tag- prefix for the new options. tag was used with idea of tagging the candidates by type. Not sure what @alexander-yakushev thinks about this.

[alexander-yakushev]

Everything looks fine, but I agree with @bbatsov about the naming. Perhaps it would be better to have a single additional option, say :extra-metadata which value should be a set, e.g. #{:doc :arglists}. This way we won't have to add new options in case at some point we need some new optional keys (unlikely, but still). Will also look a little more compact too.

[bbatsov]

Sounds good.

On 23 April 2015 at 18:51, Alexander Yakushev [email protected]
wrote:

Everything looks fine, but I agree with @bbatsov
https://github.com/bbatsov about the naming. Perhaps it would be better
to have a single additional option, say :extra-metadata which value
should be a set, e.g. #{:doc :arglists}. This way we won't have to add
new options in case at some point we need some new optional keys (unlikely,
but still). Will also look a little more compact too.

—
Reply to this email directly or view it on GitHub
#26 (comment)
.

Best Regards,
Bozhidar Batsov

http://www.batsov.com

[Deraen]

Changes implemented. Also added assoc-if util which I think makes the code easier to understand than cond->. Cond-> is fine.

[alexander-yakushev]

Great! One last thing I would ask you to do is to squash and force-push the commits into one or two or three, whichever seems more fit. Sorry for being so demanding:)

[Deraen]

And done. I would have demanded the same if this was my project :)

[alexander-yakushev]

Our buddy Travis says all is well and I can't deny it!

[alexander-yakushev]

Cast @laurentpetit.

[laurentpetit]

@alexander-yakushev many thanks for the mention!

[alexander-yakushev]

Yeah, I forgot to say thanks to @Deraen for implementing this and bearing with me and Bozhidar ;)