Fix small inspector errors and improve tests

[sanjayl]

Before submitting a PR make sure the following things have been done:

• The commits are consistent with our contribution guidelines
• You've added tests to cover your change(s)
• All tests are passing
• The new code is not generating reflection warnings
• [n/a] You've updated the readme (if adding/changing middleware)

Thanks!

A couple of small bugs/inconsistencies in the inspector middleware.

1 - Setting the page size on the fly while using the inspector was not
working. Emacs was sending an integer to Cider, which was expecting a
string, causing a type conversion error (the tests were sending a
string, which is why it wasn't caught before). Also, the user can pass
in negative integers or a zero which would break the system, so added a
sanity check.

2 - Similarly, the push op expects a stringified integer in the :idx
slot. The elisp code was converting the supplied integer to a string
prior to sending it over the wire, so there was no error in the code,
but the inconsistencies suggests we should either be more liberal with
our input types or have strict preconditions to catch these errors earlier.

To address the above, added a parse-int function to convert the
argument as needed. Perhaps this should be moved to a util namespace?

Finally, added error handling try/catch blocks around all the messaging
operations. Added tests to exercise these.

[Malabarba]

This pattern is used so many times we should refactoring it into a macro. Something like with-exception-as-error-messages.

[bbatsov]

True. But this is orthogonal to the current PR.

[Malabarba]

Is this based on current master? Some of the test builds which are failing here are passing on master. See build 11 for instance.

[bbatsov]

suggests we should either be more liberal with
our input types or have strict preconditions to catch these errors earlier.

I think we should aim for strict preconditions. Sending a string in place of a number doesn't make much sense to me.

[sanjayl]

Is this based on current master? Some of the test builds which are failing here are passing on master. See build 11 for instance.

Yes, it's rebased on the latest. Running with the master profile on my box worked OK, perhaps the Clojure snapshot had a short lived bug in it? Might try re-launching Travis to see if the error in run 11 goes away by itself?

This pattern is used so many times we should refactoring it into a macro. Something like with-exception-as-error-messages

Yeah, I've been thinking about the same thing too.

Initially, I thought about something at the level of the <op-name>-reply functions as you highlighted, but I think that would still lead to a substantial amount of boilerplate and it won't ensure that new ops follow best practices. It does have the benefit of being pretty flexible if we need to do something out of the ordinary.

Currently, I'm considering placing the safe transport wrapper at the level of the wrap-<middleware-name> function. Pro of putting it here: would reduce boilerplate and ensure all subsequent ops are safely wrapped. Con of putting it there: would not allow for custom error handling unless the code complexity is increased. Also was thinking about doing it with function passing rather than macros to make it more transparent and easier to debug and test against. Thinking about something along these lines (pseudocode in second block):

;;OLD STYLE, exceptions handled at a low level
(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 wrap-inspect
  [handler]
  (fn [{:keys [op] :as msg}]
    (case op
      "eval" (eval-reply handler msg)
      "inspect-pop" (pop-reply msg)
      "inspect-push" (push-reply msg)
      "inspect-refresh" (refresh-reply msg)
      "inspect-next-page" (next-page-reply msg)
      "inspect-prev-page" (prev-page-reply msg)
      "inspect-set-page-size" (set-page-size-reply msg)
      (handler msg))))

Would change to

;;NEW STYLE - exceptions bubble up to higher level. N.B., this is just off-the-cuff pseudocode
;;so there's probably a ton of errors here, didn't even try to compile it

(defn pop-reply [msg]
  (-> msg
      (swap-inspector! inspect/up)
      :rendered))

(defn with-safe-transport
"This will safely handle all the transport calls mapped out in the `wrap-inspect` 
function below and would likely live in some util namespace. Takes the default 
pass-through handler as well as a list of pairings between ops and the function 
calls used to create replies for those ops."
 [pass-through & pairings]
  (fn [{:keys [op transport] :as msg}]
    (if-let [reply (get (apply hash-map pairings) op)]
      (try (transport/send transport msg
                           :value (reply msg)
                           :status :done)
        (catch Exception e
          (transport/send
           transport
           (response-for msg (u/err-info e (keyword (str op "-error")))))))
      (pass-through msg))))

(defn wrap-inspect
  [handler]
  (with-safe-transport handler
    "eval" #(eval-reply handler %)
    "inspect-pop" pop-reply
    "inspect-push" push-reply
    "inspect-refresh" refresh-reply
    "inspect-next-page" next-page-reply
    "inspect-prev-page" prev-page-reply
    "inspect-set-page-size" set-page-size-reply))

Forcing all the reply functions to accept msg as an argument looks kinda crappy, but I thought it was a small tradeoff.

The lack of custom error handling is the other downside. I have some pretty simple to implement ideas on how to fix this, but I thought it would complicate things at this early stage, and I'm not sure if we actually need that capability anywhere.

Let me know what you're thoughts are. I'm off from work for the next couple days so I'll have some time to try out the different options.

[bbatsov]

Similarly, the push op expects a stringified integer in the :idx
slot.

I'd say this was a mistake. Let's simply expect a number.

[bbatsov]

As mentioned in the PR - it doesn't make sense to expect a string here. Let's just expect a number and change the related elisp code as well.

[sanjayl]

OK, I'll take out the type conversions and start putting in pre-condition checks starting with the inspect middleware. After I get more coverage tests done I can go back and make those changes on the middlewares I've already covered.

Just as a heads up, the changes in the inspect middleware will likely lead to a few erroneous inspector push related bug reports -- lein'll pull down the new cider-nrepl code expecting an integer while the old cider elisp code will still pass a string.

[bbatsov]

👍