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

Set IOContext for redirected stdout/stderr #2824

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Conversation

danielwe
Copy link
Contributor

Fixes #2823

No tests yet

Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

fonsp commented Feb 26, 2024

Thanks! Can you take a look at #2727 and try to find a solution that works for both use cases?

@danielwe
Copy link
Contributor Author

How about like this?

@fonsp
Copy link
Owner

fonsp commented Feb 27, 2024

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
Copy link
Owner

fonsp commented Feb 27, 2024

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
Copy link
Owner

fonsp commented Feb 27, 2024

PS Thanks again! Nice implementation :)

@fonsp
Copy link
Owner

fonsp commented Mar 13, 2024

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

@danielwe
Copy link
Contributor Author

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 fonsp marked this pull request as draft March 26, 2024 13:17
@fonsp fonsp added enhancement New feature or request good first issue Good for newcomers backend Concerning the julia server and runtime labels Aug 10, 2024
@fonsp
Copy link
Owner

fonsp commented Aug 10, 2024

anyone should feel free to finish this! just fork the fork :)

@ReubenJ
Copy link
Contributor

ReubenJ commented Aug 12, 2024

I've created some end-to-end style tests to cover the cases you mentioned, @fonsp. What's the best way to incorporate these? Just open a new PR?

@danielwe
Copy link
Contributor Author

I just gave you write access to my fork, @ReubenJ, so you can continue using this PR if you wish. Thanks so much for taking this on! When I said "next week" I must have meant next year...

@danielwe
Copy link
Contributor Author

Just push your commits to the stdoutiocontext branch in my fork

@fonsp
Copy link
Owner

fonsp commented Aug 13, 2024

Nice! Could you test all three forms? show(bigarray) and show(stdout, bigarray) and show(stdout, "text/plain", bigarray)

- ANSI color output when using `printstyled`

- Correct output when using `show` and `display`
with long output.
@ReubenJ
Copy link
Contributor

ReubenJ commented Aug 13, 2024

Looks like just one of the Throttled tests is failing. Should be all set otherwise!

@fonsp fonsp marked this pull request as ready for review August 14, 2024 15:05
@fonsp
Copy link
Owner

fonsp commented Aug 14, 2024

Thanks @ReubenJ, amazing!!

Looking forward to your next contribution! Maybe #2417 ? :)

@fonsp fonsp merged commit 73e9250 into fonsp:main Aug 14, 2024
13 of 14 checks passed
@ReubenJ
Copy link
Contributor

ReubenJ commented Aug 14, 2024

Sure thing. I won't have time until a couple of weeks from now, but I'd be happy to then.

@danielwe
Copy link
Contributor Author

Fantastic! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Concerning the julia server and runtime enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IOContext not properly set for stdout, disables ANSI colors
3 participants