Ensure pprint is after load-file

[SevereOverfl0w]

load-file can produce an eval message. Non-deterministically load-file
was before/after pprint middleware. This would mean that pprint would
sometimes work for load-file, and not other times, dependent upon a JVM
restart.

This makes tpope/vim-fireplace#301 work

• 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
• You've updated the readme (if adding/changing middleware)

As an aside: I can't get the tests to pass locally even on master, so I'm depending on the CI

[SevereOverfl0w]

I rewrote the implementation because of some strange bug.

I've got the test running in almost all platforms, for some reason one in the matrix is failing. I have no idea why.

Works fine locally with the same command. Worked fine with a debug commit added. I have no idea.

[bbatsov]

What was wrong with the old implementation?

[bbatsov]

In particular - why aren't we using the standard dep mechanisms of nREPL?

[bbatsov]

I'm not sure I understand what this is supposed to do. The op is one of the two?

[SevereOverfl0w]

Attempting to use the dependency mechanism triggered this stacktrace which eluded my best efforts to debug it. This is probably the better thing to get eyes on: https://travis-ci.org/clojure-emacs/cider-nrepl/jobs/257670958 Yes, this says if it's eval or load file, it works because of the delegation. If load file is before pprint, it will create an evaluation and pprint will be none the wiser. If it is after pprint, we handle it anyway. This is absolutely a hack and not the preferred solution.

…

On 31 Jul 2017 7:41 am, "Bozhidar Batsov" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/cider/nrepl/middleware/pprint.clj <#432 (comment)> : > @@ -120,7 +120,7 @@ false, or string representations of the above)." [handler] (fn [{:keys [op pprint] :as msg}] - (handler (if (and pprint (= op "eval")) + (handler (if (and pprint (#{"eval" "load-file"} op)) I'm not sure I understand what this is supposed to do. The op is one of the two? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#432 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF2h2o0aXfVen--yAyEHiBXm6ozIrBJwks5sTXcygaJpZM4Ojvyt> .

[bbatsov]

Sorry for the slow turnaround here, but I've been beyond busy at work lately.

As I want to cut a new bugfix release soon I see two options.

1. You add some explanation to the current workaround so we would not forget to address it
2. We find someone to help shed light on the original problem //cc @vspinu @Malabarba @cemerick

[vspinu]

Out of curiosity I have tried locally

modified   src/cider/nrepl/middleware/pprint.clj
@@ -127,7 +127,7 @@
 (set-descriptor!
  #'wrap-pprint
  (cljs/expects-piggieback
-  {:requires #{"clone" #'pr-values #'wrap-pprint-fn}
+  {:requires #{"clone" "load-file" #'pr-values #'wrap-pprint-fn}
    :expects #{"eval"}
    :handles
    {"pprint-middleware"

and it does not trigger any eval errors or test errors.

But I am missing the motivation for this. How is exactly pprint related to load-file? @SevereOverfl0w, is there a specific use case what you have in mind and haven't described in the OP?

As to the "one test failing" is travis choking on cider's env matrix. That happens more often than not recently.

[SevereOverfl0w]

@vspinu Here is the failure build for using that change: https://travis-ci.org/clojure-emacs/cider-nrepl/builds/257670955 If you think that is okay to pass, I can revert to that.

The use-case is that vim-fireplace sends a load-file op for some evaluation contexts. This allows that context to have pprint as a result of that evaluation.

Currently it works 50% of the time depending on the random ordering. I have a PR open here to add pprint to vim: tpope/vim-fireplace#301

[vspinu]

Here is the failure build for using that change

I see, only cljs tests are failing. I missed that part.

vim-fireplace sends a load-file op for some evaluation contexts.

But then your current PR is the right thing and the previous version was not that meaningful.

If my understanding is correct, you want the pprint to expect load-file down the pipeline just like it expects eval op, then check if either eval or load-file is requited and alter the transport. Right?

If so, then your current PR is the right step, but you then have to add "load-file" to :expects set. Otherwise it still could happen that load-file appears earlier in the pipeline, not solving your original issue.

modified   src/cider/nrepl/middleware/pprint.clj
@@ -120,7 +120,7 @@
   false, or string representations of the above)."
   [handler]
   (fn [{:keys [op pprint] :as msg}]
-    (handler (if (and pprint (= op "eval"))
+    (handler (if (and pprint (#{"eval" "load-file"} op))
                (assoc msg :transport (pprint-transport msg))
                msg))))
 
@@ -128,7 +128,7 @@
  #'wrap-pprint
  (cljs/expects-piggieback
   {:requires #{"clone" #'pr-values #'wrap-pprint-fn}
-   :expects #{"eval"}
+   :expects #{"eval" "load-file"}
    :handles
    {"pprint-middleware"
     {:doc "Enhances the `eval` op by adding pretty-printing. Not an op in itself."

[SevereOverfl0w]

I didn't think to try both, after my pain with the former. I took advantage of the fact that the docstring for load file specifically mentions eval as part of the api. I will try both this evening and see if they work together okay, or create more failures.

…

On 10 Aug 2017 17:48, "Vitalie Spinu" ***@***.***> wrote: Here is the failure build for using that change I see, only cljs tests are failing. I missed that part. vim-fireplace sends a load-file op for some evaluation contexts. But then your current PR is the right thing and the previous version was not that meaningful. If my understanding is correct, then you want the pprint to *expect* load-file down the pipeline just like it *expects* eval op, then check if either eval or load-file is requited and alter the transport. Right? If so, then your current PR is the right step, but you then have to add "load-file" to :expects set. Otherwise it still could happen that load-file appears earlier in the pipeline, not solving your original issue. modified src/cider/nrepl/middleware/pprint.clj@@ -120,7 +120,7 @@ false, or string representations of the above)." [handler] (fn [{:keys [op pprint] :as msg}]- (handler (if (and pprint (= op "eval"))+ (handler (if (and pprint (#{"eval" "load-file"} op)) (assoc msg :transport (pprint-transport msg)) msg)))) @@ -128,7 +128,7 @@ #'wrap-pprint (cljs/expects-piggieback {:requires #{"clone" #'pr-values #'wrap-pprint-fn}- :expects #{"eval"}+ :expects #{"eval" "load-file"} :handles {"pprint-middleware" {:doc "Enhances the `eval` op by adding pretty-printing. Not an op in itself." — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#432 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF2h2s8GX1WHFcniUcs5Ltjo_2rUoNcqks5sWzREgaJpZM4Ojvyt> .

[SevereOverfl0w]

Failing test passes locally. Could this be confirmed by another or rerun on the CI? (I have a history of getting the wrong test results)

…

On 10 Aug 2017 5:51 pm, "Dominic Monroe" ***@***.***> wrote: I didn't think to try both, after my pain with the former. I took advantage of the fact that the docstring for load file specifically mentions eval as part of the api. I will try both this evening and see if they work together okay, or create more failures. On 10 Aug 2017 17:48, "Vitalie Spinu" ***@***.***> wrote: > Here is the failure build for using that change > > I see, only cljs tests are failing. I missed that part. > > vim-fireplace sends a load-file op for some evaluation contexts. > > But then your current PR is the right thing and the previous version was > not that meaningful. > > If my understanding is correct, then you want the pprint to *expect* > load-file down the pipeline just like it *expects* eval op, then check > if either eval or load-file is requited and alter the transport. Right? > > If so, then your current PR is the right step, but you then have to add > "load-file" to :expects set. Otherwise it still could happen that > load-file appears earlier in the pipeline, not solving your original > issue. > > modified src/cider/nrepl/middleware/pprint.clj@@ -120,7 +120,7 @@ > false, or string representations of the above)." > [handler] > (fn [{:keys [op pprint] :as msg}]- (handler (if (and pprint (= op "eval"))+ (handler (if (and pprint (#{"eval" "load-file"} op)) > (assoc msg :transport (pprint-transport msg)) > msg)))) > @@ -128,7 +128,7 @@ > #'wrap-pprint > (cljs/expects-piggieback > {:requires #{"clone" #'pr-values #'wrap-pprint-fn}- :expects #{"eval"}+ :expects #{"eval" "load-file"} > :handles > {"pprint-middleware" > {:doc "Enhances the `eval` op by adding pretty-printing. Not an op in itself." > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#432 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AF2h2s8GX1WHFcniUcs5Ltjo_2rUoNcqks5sWzREgaJpZM4Ojvyt> > . >

[vspinu]

It's a irrelevant failure. Works locally for me. One of the project members could re-run the job.