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

Output to each session's *err* in the same manner as *out* #387

Merged
merged 3 commits into from
Jan 2, 2017

Conversation

bhagany
Copy link
Contributor

@bhagany bhagany commented Jan 1, 2017

This builds on @Malabarba's work in #304 in order to fix clojure-emacs/cider#1588 and clojure-emacs/cider#1896. In addition, this also redirects Java's System/out and System/err to *out* and *err* (they're confusingly not the same) so that everything you'd expect gets printed at the repl.

I didn't add tests because I'm not sure how to go about doing that without actually spinning up an nrepl server. I'm glad to accept guidance on this, though. Existing tests do pass.

…System/out to *out* and System/err to *err*.
@bhagany
Copy link
Contributor Author

bhagany commented Jan 1, 2017

I don't quite understand the Travis failure, but if there's anything I should do about that, let me know.

;; `PrintStream` (or maybe just not use a `PrintWriter` above).
;; (System/setOut (PrintStream. o))
(alter-var-root #'*out* (constantly o))))
(let [ow (forking-printer (vals new-state) :out)
Copy link
Member

Choose a reason for hiding this comment

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

Guess we can come up with better names than ow and ew. :-)

Copy link
Contributor Author

@bhagany bhagany Jan 1, 2017

Choose a reason for hiding this comment

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

haha, okay. I was trying to follow the existing conventions, which seemed to favor short names in this file. Plus, ow and ew made me chuckle. I'll use more descriptive names.

(PrintStream. (proxy [OutputStream] []
(close [] (.flush ^OutputStream this))
(write
([b]
Copy link
Member

Choose a reason for hiding this comment

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

Does b stand for bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - I can change that too

@bbatsov
Copy link
Member

bbatsov commented Jan 1, 2017

You should also update the namespace's description to reflect the current behaviour of the code.

I'm assuming you've tested those changes locally and they work, right?

@bhagany
Copy link
Contributor Author

bhagany commented Jan 1, 2017

Okay, will do. And yes, it works locally :)

@bhagany
Copy link
Contributor Author

bhagany commented Jan 1, 2017

I fixed the things you requested, but I also discovered an encoding issue. Trying to get to the bottom of that.

@bhagany
Copy link
Contributor Author

bhagany commented Jan 2, 2017

And, fixed! I'm unaware of any remaining issues.

@oliyh
Copy link

oliyh commented Jan 3, 2017

Hi @bhagany,

I think this has resulted in some undesired behaviour for us. We’re using clojure.tools.logging with logback and the ConsoleAppender.

Our logs, which used to go to *nrepl-server* only, now go to *cider-repl* as well.

Here are our logging deps:

[org.clojure/tools.logging "0.3.1"]
[org.slf4j/jul-to-slf4j "1.7.22"]
[ch.qos.logback/logback-classic "1.1.8" :exclusions [org.slf4j/slf4j-api]]

logback.xml:

<configuration scan="true" scanPeriod="10 seconds">

  <!-- Console output -->
  <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">

    <!-- encoder defaults to ch.qos.logback.classic.encoder.PatternLayoutEncoder -->
    <encoder>
      <pattern>%date{yyyy-MM-dd HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg severity=%level, namespace=%logger, %mdc%n</pattern>
    </encoder>
  </appender>

  <root level="INFO">
    <appender-ref ref="STDOUT" />
  </root>

</configuration>

And the behaviour we see is:

user> (clojure.tools.logging/warn "hi")
2017-01-03 13:38:23.432 [nREPL-worker-16] WARN  user - hi severity=WARN, namespace=user,
nil

user> (println "hi")
hi
nil

We only expect println to go to the REPL and all log statements to go to *nrepl-server* only. We’re being flooded by our own log messages now in the REPL.

Can you suggest a workaround or change to accommodate our use case?

Thanks

@bbatsov
Copy link
Member

bbatsov commented Jan 3, 2017

We only expect println to go to the REPL and all log statements to go to nrepl-server only. We’re being flooded by our own log messages now in the REPL.

This was always a bug and I cautioned people not to rely on this. There's a ticket on CIDER's side about redirecting certain type of output to a specific buffer, but it's been on hold for a while. Guess now we'll finally have to implement it. This is the ticket - clojure-emacs/cider#1525 It's client-side work only and should be trivial to implement. Unfortunately I'm kinda busy right now, so ideally we'd find some volunteer to help.

@bhagany
Copy link
Contributor Author

bhagany commented Jan 3, 2017

Hi @oliyh,

The fastest thing you can do is probably downgrade CIDER... that's obviously temporary though. The things that come to mind longer term are logging to a file instead of stdout, or configuring logging to be quieter on dev.

@bbatsov
Copy link
Member

bbatsov commented Jan 6, 2017

@oliyh See also clojure-emacs/cider#1907

There's a strong chance I'll reject this functionality in the end given some of the comments, but you can join the discussion. I think that it's best to just log to file and tail it within Emacs.

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.

stderr output is not printed to repl
3 participants