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

Allow pass-through of output #20

Merged
merged 13 commits into from Jan 15, 2024
Merged

Conversation

goerz
Copy link
Member

@goerz goerz commented Jan 10, 2024

Allow to capture stdout without disturbing normal output.

Closes #19

src/IOCapture.jl Outdated Show resolved Hide resolved
src/IOCapture.jl Outdated Show resolved Hide resolved
Copy link
Member

@MichaelHatherly MichaelHatherly left a comment

Choose a reason for hiding this comment

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

Cool, thanks! The name seems fine to me, agreed that "tee" is too jargony.

@goerz
Copy link
Member Author

goerz commented Jan 11, 2024

Yes, agreed :-) How's pass_through, which I just added in the last commit?

@MichaelHatherly
Copy link
Member

How's pass_through, which I just added in the last commit?

Probably fine. @mortenpi any other naming suggestions?

src/IOCapture.jl Outdated Show resolved Hide resolved
@goerz goerz marked this pull request as ready for review January 12, 2024 02:40
@goerz
Copy link
Member Author

goerz commented Jan 12, 2024

@mortenpi This should be ready to merge (if you're okay with pass_through for the name of the keyword argument)

@goerz goerz mentioned this pull request Jan 12, 2024
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

This looks great, just a few bikesheds! Do you mind adding a note into CHANGELOG.md as well?

src/IOCapture.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
goerz and others added 3 commits January 14, 2024 23:09
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
@goerz
Copy link
Member Author

goerz commented Jan 15, 2024

All done!

@mortenpi
Copy link
Member

One thing I noticed is that it strips the ASCII colors from the passthrough:

image

I wonder if some IOContext shenanigans would be sufficient here. But happy to merge this as is though, with this as a "known limitation" though.

@mortenpi
Copy link
Member

mortenpi commented Jan 15, 2024

Oh, it does work if you pass color = true as well:

image

I guess the question is whether we want the passthrough to be consistent with the "input" or the captured output. Both are pretty reasonable behaviors. If we go with the latter (i.e. current behavior), I only that this interaction with color should be documented.

@goerz
Copy link
Member Author

goerz commented Jan 15, 2024

Good point! It wasn't really something I considered. The current behavior seems ok to me. Indeed, the passthrough is always consistent with the captured output, also in that stdout and stderr are merged. I've added a sentence to clarify that and the color behavior.

@goerz
Copy link
Member Author

goerz commented Jan 15, 2024

Can I get away with not adding an explicit test for the color behavior? ;-)

I've added a test for the color behavior as well

@mortenpi
Copy link
Member

Can I get away with not adding an explicit test for the color behavior? ;-)

I've added a test for the color behavior as well

I took the liberty of slightly modifying the tests (adding a stdout/err merging test too, basically, and explicitly checking the color codes).

@mortenpi
Copy link
Member

Ugh. I guess the real question is whether we need to support 1.0..

no method matching redirect_stdout(::IOContext{IOStream})

@goerz
Copy link
Member Author

goerz commented Jan 15, 2024

Yeah, I was just fixing that… I think it's just the test that doesn't run on 1.0. The feature itself should be fine.

I'm also okay with testing for the exact escape code, I just wasn't quite sure if they're the same on Windows/Unix, hence just the test for any escape code in the last iteration.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Yea, restricting the test to 1.6+ seems fine as well!

@goerz
Copy link
Member Author

goerz commented Jan 15, 2024

I just tested in manually on Julia 1.0. It doesn't crash, but it seems like color=true just has no effect. It's getting a little late for me to hunt that down, though. It seems like such an edge case that I'd be willing to just ignore it.

@mortenpi
Copy link
Member

It doesn't crash, but it seems like color=true just has no effect. It's getting a little late for me to hunt that down, though. It seems like such an edge case that I'd be willing to just ignore it.

That is a known, documented limitation 😄

IOCapture.jl/README.md

Lines 64 to 69 in cea2050

### ANSI color / escape sequences
On Julia 1.5 and earlier, setting `color` to `true` has no effect, because the [ability to
set `IOContext` attributes on redirected streams was added in
1.6](https://github.com/JuliaLang/julia/pull/36688). I.e. on those older Julia versions the
captured output will generally not contain ANSI color escape sequences.

@mortenpi mortenpi merged commit ec43c7f into JuliaDocs:master Jan 15, 2024
17 checks passed
@goerz goerz deleted the mg/tee-option branch January 15, 2024 15:09
@goerz goerz mentioned this pull request Jan 16, 2024
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.

Add a pass_through option
3 participants