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

explicitly propagate displaysize in sprint #42660

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Oct 15, 2021

Fixes #42649, closes #34721

julia> displaysize(stdout)
(20, 68)

julia> displaysize(IOContext(stdout))
(20, 68)

julia> sprint(; context=stdout) do io
           print(io, displaysize(io))
       end
"(20, 68)"

This also fixes a bug in Pkg where the progress bar has support for shrinking its size when the terminal gets too narrow, but Pkg.precompile uses sprint so it doesn't work there.

It is a bit unclear where to add a test for this...

base/strings/io.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Sponsor Member

timholy commented Oct 18, 2021

This seems to fix my concern about bugs. My remaining concern is the scope of this change: if we're changing it in sprint rather than in unwrapcontext, presumably that means we're trying to preserve some of #42649 (comment). But how do we decide which ones should change and which ones should stay as-is? Why is sprint different from other uses? Isn't it kind of the poster child for #42649 (comment)?

I think a better fix for this would be to add a keyword to IOContext(io::IO, context::IO) that controls whether size gets copied, and then pass-through keyword variants of Base functions that call IOContext(io, ctx). That, plus documentation clarifying what happens by default and what can be enabled.

@KristofferC
Copy link
Sponsor Member Author

That makes sense, I tried to move it to unwrapcontext but running into a bit of a bootstrap problem where displaysize(io::IO) is not defined at that point and the system tries to use this method before it ends up getting defined. And just moving the definition of displaysize(io::IO) up doesn't really work either because it uses ENV which is also not defined. Hm.

@kshyatt kshyatt added the display and printing Aesthetics and correctness of printed representations of objects. label Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

displaysize is not inherited from tty inIOContext(io, tty) sprint(; context=io) loses displaysize of io
3 participants