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

fix: remove the possible duplicate output introduced in a052dd8 #737

Merged

Conversation

hugoduncan
Copy link
Member

Output to the original output outside of the with-out-binding loop.

Output to the original output outside of the with-out-binding loop.
@vemv
Copy link
Member

vemv commented Dec 9, 2021

Is it possible to add test coverage that would have caught this bug?

(defn- dispatch-string
([messages type ^String x]
(.write ^Writer (original-output type) x)
(with-out-binding [printer messages type]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vemv
Copy link
Member

vemv commented Dec 10, 2021

Btw, from what I've seen it's fine to use a branch within this repo, instead of a fork. Makes it easier to review locally

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified locally that if reverting the fix, the included tests would fail with duplicates as intended:

FAIL in (forking-printer-test) (out_test.clj:68)
forking-printer prints to all message streams and original stream with String argument
expected: (= "Hello" (.toString out-writer))
  actual: (not (= "Hello" "HelloHello"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:73)
forking-printer prints to all message streams and original stream with String argument
expected: (= "HelloHello" (.toString out-writer))
  actual: (not (= "HelloHello" "HelloHelloHelloHello"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:84)
forking-printer prints to all message streams and original stream with int
expected: (= " " (.toString out-writer))
  actual: (not (= " " "  "))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:89)
forking-printer prints to all message streams and original stream with int
expected: (= "  " (.toString out-writer))
  actual: (not (= "  " "    "))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:99)
forking-printer prints to all message streams and original stream with char array
expected: (= "and" (.toString out-writer))
  actual: (not (= "and" "andand"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:104)
forking-printer prints to all message streams and original stream with char array
expected: (= "andand" (.toString out-writer))
  actual: (not (= "andand" "andandandand"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:114)
forking-printer prints to all message streams and original stream with String with offsets
expected: (= "good" (.toString out-writer))
  actual: (not (= "good" "goodgood"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:119)
forking-printer prints to all message streams and original stream with String with offsets
expected: (= "goodgood" (.toString out-writer))
  actual: (not (= "goodgood" "goodgoodgoodgood"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:129)
forking-printer prints to all message streams and original stream with char array with offsets
expected: (= "bye" (.toString out-writer))
  actual: (not (= "bye" "byebye"))

lein test :only cider.nrepl.middleware.out-test/forking-printer-test

FAIL in (forking-printer-test) (out_test.clj:134)
forking-printer prints to all message streams and original stream with char array with offsets
expected: (= "byebye" (.toString out-writer))
  actual: (not (= "byebye" "byebyebyebye"))

@bbatsov bbatsov merged commit df6b5c6 into clojure-emacs:master Dec 10, 2021
@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

Thanks!

@vemv
Copy link
Member

vemv commented Dec 10, 2021

@bbatsov : the bug being fixed seemed pretty bad, so it might be a good idea to cut a bugfix release now?

@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

It seems to me that only a tiny number of people have been affected by this, so it can wait until the scheduled release on Sunday. I haven't seen any reports of the problem at all - after all this only affects output that would normally end up in the server buffer, not regular output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants