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

Add color keyword argument #1

Merged
merged 3 commits into from
Oct 27, 2020
Merged

Add color keyword argument #1

merged 3 commits into from
Oct 27, 2020

Conversation

kimikage
Copy link
Contributor

This is a change intended for Documenter's ANSI color support (cf. JuliaDocs/Documenter.jl#1441)

This adds color keyword argument to iocapture(). Currently, this argument does not work on Julia v1.5 or earlier, but to avoid version-based conditional branching on the user side, it does not throw an error.

Of course, it is a reasonable option to set color to false by default. However, I chose true for the following reasons.

  • It is intuitive that the raw stdout output matches the capture result.
  • The ANSI escape codes are easy to detect and remove, but impossible to add later.
  • Since Julia v1.6 is still in development, this breaking change has little problem.
    • However, we need to bump a minor version as it is not compatible with v0.1.

This adds `color` keyword argument to `iocapture()`.
Currently, this argument does not work on Julia v1.5 or earlier,
but to avoid version-based conditional branching on the user side,
it does not throw an error.
@mortenpi mortenpi added the enhancement New feature or request label Oct 25, 2020
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.

Sorry for the delay. The implementation and tests look great, thanks a lot!

  1. I wonder if the default to color should be false though. My argument would be that the most common use case of iocapture is probably where you want to do some post-processing on the output (e.g. to regex out some information). At that point, you probably prefer not having the color escapes in the output. To counter the intuitiveness point, I think for me it would be slightly more intuitive if the captured output (by default) only contains the characters that I can see in the terminal.

    It would also make the default behavior consistent across Julia versions, with the ability to capture color, that must be explicitly enabled, only having an effect in 1.6+.

    Do these arguments sway you, or do you think true is still the better option?

  2. Currently, if I read the tests right, we test slightly different code paths depending on settings (e.g. depending on --color or Julia version)? I wonder if we could have cases where we force-enable and force-disable color on stdout/err somehow, to make sure we test all the cases every time? It's not too important though, i.e. not worth investing too much time into this I think.

src/IOCapture.jl Outdated
redirect_stderr(pipe.in)
if VERSION >= v"1.6.0-DEV.481" # https://github.com/JuliaLang/julia/pull/36688
pe_stdout = IOContext(pipe.in, :color => get(stdout, :color, false) & color)
pe_stderr = IOContext(pipe.in, :color => get(stdout, :color, false) & color)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be get(stderr, ...)?

@MichaelHatherly
Copy link
Member

It would also make the default behavior consistent across Julia versions, with the ability to capture color, that must be explicitly enabled, only having an effect in 1.6+.

Agreed, if we can have consistent behavior across versions that would be less surprising.

src/IOCapture.jl Outdated
@@ -62,10 +66,17 @@ function iocapture(f; throwerrors::Union{Bool,Symbol}=true)
# Redirect both the `stdout` and `stderr` streams to a single `Pipe` object.
pipe = Pipe()
Base.link_pipe!(pipe; reader_supports_async = true, writer_supports_async = true)
redirect_stdout(pipe.in)
redirect_stderr(pipe.in)
if VERSION >= v"1.6.0-DEV.481" # https://github.com/JuliaLang/julia/pull/36688
Copy link
Member

Choose a reason for hiding this comment

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

Worth sticking a @static in here to avoid the runtime branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm skeptical that there is a runtime branch, but I'll stick the @static to be clear.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, looks like it's smart enough these days:

julia> function f(x, y)
           if VERSION >= v"1.4.0"
               x + y
           else
               x - y
           end
       end

julia> @code_typed f(1, 2)
CodeInfo(
1 ─ %1 = Base.add_int(x, y)::Int64
└──      return %1
) => Int64

No point adding @static then.

This also fixes a typo (stdout --> stderr).
@kimikage
Copy link
Contributor Author

Thanks for the reviews!
I changed the default value to false.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Morten Piibeleht <[email protected]>
@mortenpi mortenpi added this to the 0.1.1 milestone Oct 27, 2020
@mortenpi mortenpi merged commit 4aa7ad4 into JuliaDocs:master Oct 27, 2020
@mortenpi
Copy link
Member

mortenpi commented Oct 27, 2020

Thanks again! Will tag this shortly, so that the Documenter PR could make use of this without having to jump through hoops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants