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

Abstract away op error handling #327

Merged
merged 1 commit into from Apr 1, 2016
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Abstract away op error handling

A new util ns in `cider.nrepl.middleware.util.error-handling` which
provides the `with-safe-transport` macro, abstracting away transport,
error handling, and stacktrace analysis. Most middlewares were updated
to use this system, and the tests were updated to reflect these changes.

In order to utilize the nicely formatted stacktraces stuffed into these
responses, concurrent changes have been made to CIDER and will be
submitted as a PR as well.
  • Loading branch information
sanjayl committed Mar 31, 2016
commit 9a81e27dcea44457b1670f4dfd8895e1e772f658
@@ -1,11 +1,9 @@
(ns cider.nrepl.middleware.apropos
"Search symbols and docs matching a regular expression"
{:author "Jeff Valk"}
(:require [clojure.string :as str]
(:require [cider.nrepl.middleware.util.error-handling :refer [with-safe-transport]]
[clojure.string :as str]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]
[clojure.tools.nrepl.misc :refer [response-for]]
[clojure.tools.nrepl.transport :as t]
[cider.nrepl.middleware.util.misc :as u]
[cider.nrepl.middleware.util.namespace :as ns]))

;;; ## Overview
@@ -82,26 +80,14 @@
(defn handle-apropos
"Return a sequence of vars whose name matches the query pattern, or if
specified, having the pattern in their docstring."
[{:keys [ns query search-ns docs? privates? case-sensitive? transport] :as msg}]
(try
(let [results (find-symbols ns query search-ns docs? privates? case-sensitive?)]
(t/send transport (response-for msg :apropos-matches results :status :done)))
(catch java.util.regex.PatternSyntaxException e
(t/send
transport
(response-for msg :status #{:done :apropos-regexp-error} :error-msg (.getMessage e))))
(catch Exception e
(t/send
transport
(response-for msg (u/err-info e :apropos-error))))))
[{:keys [ns query search-ns docs? privates? case-sensitive?] :as msg}]
{:apropos-matches (find-symbols ns query search-ns docs? privates? case-sensitive?)})

(defn wrap-apropos
"Middleware that handles apropos requests"
[handler]
(fn [{:keys [op] :as msg}]
(if (= op "apropos")
(handle-apropos msg)
(handler msg))))
(with-safe-transport handler
"apropos" handle-apropos))

;; nREPL middleware descriptor info
(set-descriptor!
@@ -1,33 +1,19 @@
(ns cider.nrepl.middleware.classpath
(:require [clojure.java.classpath :as cp]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]
[clojure.tools.nrepl.misc :refer [response-for]]
[clojure.tools.nrepl.transport :as transport]
[cider.nrepl.middleware.util.misc :as u]))
(:require [cider.nrepl.middleware.util.error-handling :refer [with-safe-transport]]
[clojure.java.classpath :as cp]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]))

(defn classpath []
(map str (cp/classpath)))

(defn classpath-reply
[{:keys [transport] :as msg}]
(try
(transport/send
transport
(response-for msg
:classpath (classpath)
:status :done))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :classpath-error))))))
(defn classpath-reply [msg]
{:classpath (classpath)})

(defn wrap-classpath
"Middleware that provides the java classpath."
[handler]
(fn [{:keys [op] :as msg}]
(if (= "classpath" op)
(classpath-reply msg)
(handler msg))))
(with-safe-transport handler
"classpath" classpath-reply))

(set-descriptor!
#'wrap-classpath
@@ -1,9 +1,8 @@
(ns cider.nrepl.middleware.complete
(:require [clojure.string :as s]
[clojure.tools.nrepl.transport :as transport]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]
[clojure.tools.nrepl.misc :refer [response-for]]
[cider.nrepl.middleware.util.cljs :as cljs]
[cider.nrepl.middleware.util.error-handling :refer [with-safe-transport]]
[cider.nrepl.middleware.util.misc :as u]
[compliment.core :as jvm-complete]
[cljs-tooling.complete :as cljs-complete]))
@@ -26,35 +25,19 @@
(when-not (cljs/grab-cljs-env msg)
(jvm-complete/documentation (str symbol) (u/as-sym ns))))

(defn complete-reply
[{:keys [transport] :as msg}]
(try
(transport/send
transport
(response-for msg :completions (complete msg) :status :done))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :completion-error))))))
(defn complete-reply [msg]
{:completions (complete msg)})

(defn doc-reply
[{:keys [transport] :as msg}]
(try
(let [results (completion-doc msg)]
(transport/send transport (response-for msg :completion-doc results :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :completion-doc-error))))))
[msg]
{:completion-doc (completion-doc msg)})

(defn wrap-complete
"Middleware that looks up possible functions for the given (partial) symbol."
[handler]
(fn [{:keys [op] :as msg}]
(cond
(= "complete" op) (complete-reply msg)
(= "complete-doc" op) (doc-reply msg)
:else (handler msg))))
(with-safe-transport handler
"complete" complete-reply
"complete-doc" doc-reply))

(set-descriptor!
#'wrap-complete
@@ -1,27 +1,16 @@
(ns cider.nrepl.middleware.format
(:refer-clojure :exclude [read-string])
(:require [cider.nrepl.middleware.util.misc :refer [err-info]]
(:require [cider.nrepl.middleware.util.error-handling :refer [with-safe-transport]]
[cider.nrepl.middleware.pprint :as pprint]
[cljfmt.core :as fmt]
[clojure.string :as string]
[clojure.tools.nrepl.transport :as transport]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]
[clojure.tools.nrepl.misc :refer [response-for]]
[clojure.tools.reader.edn :as edn]
[clojure.tools.reader.reader-types :as readers]))

(defn format-code-reply
[{:keys [transport code] :as msg}]
(try
(transport/send
transport
(response-for msg
:formatted-code (fmt/reformat-string code)
:status :done))
(catch Exception e
(transport/send
transport
(response-for msg (err-info e :format-code-error))))))
[{:keys [code] :as msg}]
{:formatted-code (fmt/reformat-string code)})

(defn- read-edn
"Returns a vector of EDN forms, read from the string s."
@@ -35,31 +24,22 @@
(recur (conj forms form)))))))

(defn- format-edn
[{:keys [edn pprint-fn] :as msg}]
[edn pprint-fn]
(->> (read-edn edn)
(map #(with-out-str (pprint-fn %)))
string/join
string/trim))

(defn format-edn-reply
[{:keys [transport] :as msg}]
(try
(transport/send
transport
(response-for msg :formatted-edn (format-edn msg) :status :done))
(catch Exception e
(transport/send
transport
(response-for msg (err-info e :edn-read-error))))))
[{:keys [edn pprint-fn] :as msg}]
{:formatted-edn (format-edn edn pprint-fn)})

(defn wrap-format
"Middleware that provides code formatting."
[handler]
(fn [{:keys [op] :as msg}]
(case op
"format-code" (format-code-reply msg)
"format-edn" (format-edn-reply msg)
(handler msg))))
(with-safe-transport handler
"format-code" format-code-reply
"format-edn" format-edn-reply))

(set-descriptor!
#'wrap-format
@@ -3,14 +3,18 @@
[clojure.java.io :as io]
[clojure.java.javadoc :as javadoc]
[cider.nrepl.middleware.util.cljs :as cljs]
[cider.nrepl.middleware.util.error-handling :refer [with-safe-transport]]
[cider.nrepl.middleware.util.java :as java]
[cider.nrepl.middleware.util.namespace :as ns]
[cider.nrepl.middleware.util.misc :as u]
[clojure.repl :as repl]
[cljs-tooling.info :as cljs-info]
[clojure.tools.nrepl.transport :as transport]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]
[clojure.tools.nrepl.misc :refer [response-for]]))
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]))

(defn- boot-project? []
;; fake.class.path under boot contains the original directories with source
;; files, see https://github.com/boot-clj/boot/issues/249
(not (nil? (System/getProperty "fake.class.path"))))

(defn- boot-class-loader
"Creates a class-loader that knows original source files paths in Boot project."
@@ -258,16 +262,10 @@
u/transform-value)))

(defn info-reply
[{:keys [transport] :as msg}]
(try
(if-let [var-info (format-response (info msg))]
(transport/send
transport (response-for msg var-info {:status :done}))
(transport/send
transport (response-for msg {:status #{:no-info :done}})))
(catch Exception e
(transport/send
transport (response-for msg (u/err-info e :info-error))))))
[msg]
(if-let [var-info (format-response (info msg))]
var-info
{:status :no-info}))

(defn extract-eldoc [info]
(cond
@@ -291,25 +289,17 @@
(format-eldoc raw-eldoc)))

(defn eldoc-reply
[{:keys [transport] :as msg}]
(try
(if-let [var-eldoc (eldoc msg)]
(transport/send
transport (response-for msg {:eldoc var-eldoc :status :done}))
(transport/send
transport (response-for msg {:status #{:no-eldoc :done}})))
(catch Exception e
(transport/send
transport (response-for msg (u/err-info e :eldoc-error))))))
[msg]
(if-let [var-eldoc (eldoc msg)]
{:eldoc var-eldoc}
{:status :no-eldoc}))

(defn wrap-info
"Middleware that looks up info for a symbol within the context of a particular namespace."
[handler]
(fn [{:keys [op] :as msg}]
(cond
(= "info" op) (info-reply msg)
(= "eldoc" op) (eldoc-reply msg)
:else (handler msg))))
(with-safe-transport handler
"info" info-reply
"eldoc" eldoc-reply))

(set-descriptor!
#'wrap-info
@@ -1,5 +1,6 @@
(ns cider.nrepl.middleware.inspect
(:require [cider.nrepl.middleware.util.cljs :as cljs]
[cider.nrepl.middleware.util.error-handling :refer [base-error-response]]
[cider.nrepl.middleware.util.inspect :as inspect]
[cider.nrepl.middleware.util.misc :as u]
[clojure.tools.nrepl.middleware :refer [set-descriptor!]]
@@ -47,77 +48,35 @@
[handler msg]
(handler (eval-msg msg)))

(defn pop-reply
[{:keys [transport] :as msg}]
(try
(let [inspector (swap-inspector! msg inspect/up)]
(transport/send
transport
(response-for msg :value (:rendered inspector) :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :inspect-pop-error))))))

(defn push-reply
[{:keys [idx transport] :as msg}]
(try
(let [inspector (swap-inspector! msg inspect/down idx)]
(transport/send
transport
(response-for msg :value (:rendered inspector) :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :inspect-push-error))))))

(defn refresh-reply
[{:keys [transport] :as msg}]
(try
(let [inspector (swap-inspector! msg #(or % (inspect/fresh)))]
(transport/send
transport
(response-for msg :value (:rendered inspector) :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :inspect-refresh-error))))))

(defn next-page-reply
[{:keys [transport] :as msg}]
(try
(let [inspector (swap-inspector! msg inspect/next-page)]
(transport/send
transport
(response-for msg :value (:rendered inspector) :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :inspect-next-page-error))))))

(defn prev-page-reply
[{:keys [transport] :as msg}]
(try
(let [inspector (swap-inspector! msg inspect/prev-page)]
(transport/send
transport
(response-for msg :value (:rendered inspector) :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :inspect-prev-page-error))))))

(defn set-page-size-reply
[{:keys [page-size transport] :as msg}]
(try
(let [inspector (swap-inspector! msg inspect/set-page-size page-size)]
(transport/send
transport
(response-for msg :value (:rendered inspector) :status :done)))
(catch Exception e
(transport/send
transport
(response-for msg (u/err-info e :inspect-set-page-size-error))))))
(defn- success [{:keys [transport] :as msg} inspector]
(transport/send transport (response-for msg :value (:rendered inspector) :status :done)))

(defn- failure [{:keys [transport] :as msg} err err-kw]
(transport/send transport (base-error-response msg err err-kw :done)))

(defn pop-reply [msg]
(try (success msg (swap-inspector! msg inspect/up))
(catch Exception e (failure msg e :inspect-pop-error))))

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

I wonder if the Elisp code cares about those different error codes. Seems to me there's still room to cleanup/simplify all those ops.

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

You're right, the Elisp code has absolutely no idea about these error codes. They could be removed entirely, but I think they're good to have so that we can "tune" the error reporting UI based on the type of error returned (which is something I've been experimenting with). Eval/big errors should certainly get the stacktrace viewer, some other types should just get a minibuffer message, and other types of errors might just get swallowed. The user would also be able to customize this by fooling around with some variables.


(defn push-reply [msg]
(try (success msg (swap-inspector! msg inspect/down (:idx msg)))
(catch Exception e (failure msg e :inspect-push-error))))

(defn refresh-reply [msg]
(try (success msg (swap-inspector! msg #(or % (inspect/fresh))))
(catch Exception e (failure msg e :inspect-refresh-error))))

(defn next-page-reply [msg]
(try (success msg (swap-inspector! msg inspect/next-page))
(catch Exception e (failure msg e :inspect-next-page-error))))

(defn prev-page-reply [msg]
(try (success msg (swap-inspector! msg inspect/prev-page))
(catch Exception e (failure msg e :inspect-prev-page-error))))

(defn set-page-size-reply [msg]
(try (success msg (swap-inspector! msg inspect/set-page-size (:page-size msg)))
(catch Exception e (failure msg e :inspect-set-page-size-error))))

(defn wrap-inspect

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

Was this not updated because of the eval in it?

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

It's more because of how the inspect-reply function handles creating a response. It writes a non-terminated response and expects the {:status #{:done}} cap to happen deeper down in the handler. The macro expects a response that can be immediately terminated. I didn't feel like mucking around with this too much because I didn't understand it well.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

I see. Probably we should refactor this down the road. It's a pretty old middleware, that hasn't be updated significantly in quite a while.

"Middleware that adds a value inspector option to the eval op. Passing a
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.