Rewrite `sync-request` subsystem

Changes to `nrepl-send-sync-request`:

   - returned response is in a standard nREPL form (:done and :value were confusing)
   - returned response is a merged dictionary of all the key-vals received so far (not just of "value", "out" and "err" keys as it was before).
   - nrepl-operation is now checked inside this function
   - when only "value" slot is requested, `err` and `out` are meaningfully handled

Other changes:

   - Reduce redundancy in eval APIs by removing several eval functions on CIDER
     side. They were either not used or could be replaced with direct calls to
     nrepl API.
   - Enforce `cider-sync-request:MILDLEWARE` convention and move all requests
     into `cider-client.el`.

-----

After grouping all requests together some question marks emerged.

-  Not all requests send the "session" argument. It still works but I assume it's an oversight. I didn't change that.  
- `macroexpand` op is special as it's imlemented as 3 separate ops "macroexpand" "macroexpand-1" "macroexpand-all". I think this is confusing. It should be one middleware with "expander" parameter.
- All requests are using `current-session`, shouldn't they be using `tooling-session` instead? Otherwise tooling session is not much used. I wonder if it could be dispensed with altogether.


Trust me on this - using value conflicts with nREPL design ideas. It’s my fault that I took this shortcut instead of altering the request handling code first. I’ll change all the middleware in the future for sure, so not being flexible about slot names is not an option. 

—
Cheers, 
Bozhidar

On September 15, 2014 at 11:52:53 PM, Vitalie Spinu (notifications@github.com) wrote:

In cider-client.el:

> +                      (cider-load-file-handler (current-buffer))))
> +
> +
> +;;; Sync Requests
> +
> +(defun cider-sync-request:apropos (query &optional search-ns docs-p privates-p case-sensitive-p)
> +  "Send \"apropos\" op with args SEARCH-NS, DOCS-P, PRIVATES-P, CASE-SENSITIVE-P."
> +  (nrepl-send-sync-request `("op" "apropos"
> +                             "ns" ,(cider-current-ns)
> +                             "query" ,query
> +                             ,@(when search-ns `("search-ns" ,search-ns))
> +                             ,@(when docs-p '("docs?" "t"))
> +                             ,@(when privates-p '("privates?" "t"))
> +                             ,@(when case-sensitive-p '("case-sensitive?" "t")))))
> +
> +(defun cider-sync-request:classpath ()
In practice every request, but eval will use the full param you introduced which makes this decision questionable (as all middleware ops should stop abusing the result slot asap).

I think using "value" slot is not bad. Most of the "ops" are returning only one value and it's not wrong to call it "value" as the eval middleware does. I would not call it an abuse.

Abusive was the previous implementation that would specifically concatenate value, err and out slots. But that's changed now and that was the main reason to rewrite the stuff in the first place.

—
Reply to this email directly or view it on GitHub.