Set IOContext for redirected stdout/stderr

danielwe commented 4 months ago

Fixes #2823

No tests yet

github-actions[bot] commented 4 months ago

Try this Pull Request!

Open Julia and type:

  julia> import Pkg
  julia> Pkg.activate(temp=true)
  julia> Pkg.add(url="https://github.com/danielwe/Pluto.jl", rev="stdoutiocontext")
  julia> using Pluto

danielwe commented 4 months ago

Tests added, loosely following the pattern in test/Logging.jl. The idea is to verify that every IOContext property set in PlutoRunner.default_stdout_iocontext is visible on stdout and stderr inside a notebook. (I included stderr because PlutoRunner sends both streams to the same pipe.) Let me know if this should be done differently.

fonsp commented 4 months ago

Thanks! Can you take a look at https://github.com/fonsp/Pluto.jl/pull/2727 and try to find a solution that works for both use cases?

danielwe commented 4 months ago

How about like this?

fonsp commented 4 months ago

Nice! This seems to match the REPL:

julia> IOContext(stdout).dict
Base.ImmutableDict{Symbol, Any} with 1 entry:
  :color => true

julia> struct ShowMyIO end

julia> Base.show(io::IO, m::MIME"text/plain", s::ShowMyIO) = @info "s" IOContext(io).dict

julia> ShowMyIO()
┌ Info: s
│   (IOContext(io)).dict =
│    Base.ImmutableDict{Symbol, Any} with 4 entries:
│      :module                => Main
│      :limit                 => true
│      :last_shown_line_infos => Tuple{String, Int64}[]
└      :color                 => true

fonsp commented 4 months ago

Could you write the tests in a more end-to-end style? Right now the test is very exact for the question: "did the IO context get set properly"? What I would prefer to test is whether the two issues are fixed:

Does printstyled work?
Does show(bigarray) and show(stdout, bigarray) and show(stdout, "text/plain", bigarray) show the full array contents? (e.g. check if the middle element occurs in the string)
Does display(bigarray) limit the display?

The benefit of an end-to-end test is:

We are free to make changes to the implementation in the future, without having to also change our tests.
We can later make (seemingly unrelated) changes somewhere else in the stdout display mechanism, with the guarantee that these two issues will remain fixed.

It would also be nice if the tests are added to the existing notebook in test/Logging.jl, this makes the tests run faster because no new process needs to be started.

You can ignore the GitHub Action test failures on Julia nightly, that's being worked on in another PR

fonsp commented 4 months ago

PS Thanks again! Nice implementation :)

fonsp commented 3 months ago

@danielwe Hey! Do you have time to take a look at the tests?

danielwe commented 3 months ago

Thanks for the feedback! I'm rather strapped for time at the moment. I can probably look at it next week, but if someone wants to adopt this to get it across the finish line sooner, feel free!

fonsp / Pluto.jl

Set IOContext for redirected stdout/stderr #2824

Try this Pull Request!