-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add capture macro for both out and err. #36
base: master
Are you sure you want to change the base?
Conversation
LGTM but needs tests |
Yeah, sure. For the time being I added the macro to my PR to PlutoUI.jl But: For making this work with PIuto I had to remove the |
I would guess it needs to work something like this instead: and temporarily set the |
src/Suppressor.jl
Outdated
logger = logstate.logger | ||
if logger.stream == original_stderr | ||
new_logstate = Base.CoreLogging.LogState(typeof(logger)(err_wr, logger.min_level)) | ||
Core.eval(Base.CoreLogging, Expr(:(=), :(_global_logstate), new_logstate)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hack seems quite ugly and fragile. It's quite likely to break in future versions as it relies on a lot of implementation details.
It also basically assumes that logger
is a Logging.ConsoleLogger
because it assumes that logger.stream
exists.
I'm not sure what the intent of checking logger.stream == original_stderr
, but other than that it would be better to create a new ConsoleLogger
, and use the public API Logging.with_logger
to install it. Then you can avoid relying on a big pile of implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is pre-Julia-1.0 code!
with_logger
makes total sense, that's probably better than using global_logger(logger)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, kind of more understandable: JuliaPluto/PlutoUI.jl#31 (comment)
However not sure if I can ignore @async ...
Why not just compose the two: macro capture(expr)
quote
local captured_out, captured_err
captured_out = @capture_out begin
captured_err = @capture_err $(esc(expr))
end
(captured_out, captured_err)
end
end |
The problem is that returning a Maybe we could return an (@capture_both begin
println(stdout, "hello")
println(stderr, "asfdasdf")
println(stdout, "world!")
end) == [
stdout => "hello",
stderr => "asdfasdf",
stdout => "world!",
] This can't be achieved by simple composition of |
Agree, see the point.
See also JuliaPluto/PlutoUI.jl@9fa7555 |
Using async read now. See the update in I think this is as close as we can come to the idea to collect stdout and stderr together into one string. Concerning the idea to collect things into an array of pairs with the information about the stream I have this (would be easy to replace the first
I did not find a way to synchronize the sequence of lines in the case there are multiple line outputs in to one of stderr and stdout. May be it is possible though. |
I researched what the
In fact this IMHO belongs to Base, but at least this could be defined here in Suppressor in order to make the code more understandable. And indeed I understand now that it may be necessary to check for this when running under generic Julia. OTOH it seems to be no problem to omit this test when running code cells in Pluto notebooks. |
* Remove pre-Julia 1.0 code, replace it with with_logger * Aocument ccalling jl_generating_output * Add additional test for @capture
Hi would be great to have this in Pluto.jl