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

introduce IOContext and ImmutableDict to fix some of show, print, & friends #13825

Merged
merged 6 commits into from
Dec 31, 2015

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Oct 30, 2015

This is a small PR is intended to provide a general framework to address quite a few of the broader issues with show, print, & friends.

The specific implementation details is that this is an immutable dictionary that is a subclass of IO (so it can be used anywhere an IO object can be used), where the dictionary allows a display function to pass along arbitrary metadata to arbitrary other parts of the printing infrastructure.

As proof-of-concept examples, the PR replaces the existing global or TLS shown_set, limit_output, type-emphasis, with a simple IO-local mechanism, plus it provides a way to add a lock to any IO object.

Just a quick search nets the following list of existing issues for reference:

solves #5709 "Show, showcompact, showall, showlimited, print, repr, summary, and friends"
addresses #7959 "proposed cleanup of output functions"
mostly implements #10794 "Show of parameterized methods" (replacing PR #10861)
provides a mechanism for addressing #6117 "array display is a mess"
alternative/complement to PR #8987 "WIP: cleanup of output functions"
addresses #10009 Overload print for Markdown Strings
fixes #11364 integrate code_warntype with the output system
fixes #10327 print_color and println_color

additionally, cross-ref with:
PR #12929 showcompact(::RefValue) prints the contained object compactly
#6921 Pager support in the repl

PR #13256 Wrapper to annotate the MIME type of some data

Inspired by https://groups.google.com/forum/#!topic/julia-dev/uKPxXCQCU_A

TODO:

@vtjnash vtjnash changed the title RFC: IOContext for fixing show, print, & friends RFC: proposal for fixing show, print, & friends with IOContext Oct 30, 2015
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Oct 30, 2015

for a usage example take a look at the implementation of tvar_env 11a5c37#diff-b4267643a238b5a375b3e055817ee6caR833 or typeemphasize. once the IOContext implementation is in place, the addition of each of these is accomplished in a small amount of code.

@StefanKarpinski
Copy link
Sponsor Member

Did I miss you explaining the high-level idea of IOContext somewhere?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 1, 2015

there's not really much to explain, so I wasn't sure what to write. the problem in all of the IO issues above is that the show methods want to customize the output based on some property known only to the caller, but there was no way to pass that information into the callee -- other than TLS variables. IOContext is unlike any other IO object (well, except maybe Process), in that it is does not actually reflect a type of IO stream, but instead can be used to convey context about how the IO stream should be used (or ignored and used as a generic stream).

@StefanKarpinski
Copy link
Sponsor Member

Start with "what is an IO context?". Move on to "why do we need them?" Probably want to include "what problems do they solve?"

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 2, 2015

what is an IO context?

an immutable Dictionary that is a subclass of IO. or alternatively, keyword arguments for all IO functions

why do we need them?

the existing problems in the show/print hierarchy (ref #5709) are generally of the form that the caller only can provide two arguments: what to print and where to print. but the caller often wants to include a third consideration: how to print (limited, recursive, types-emphasized, typevars ignored, mime formatting).

what problems do they solve?

this PR provides a mechanism to essentially add keyword arguments to every function ever written for IO, without changing a single line of existing code (by transparently reusing the existing IO argument)


limit_output(::ANY) = false
limit_output(io::IOContext) = io.limit_output
function in{I<:IOContext}(value, io_key::Pair{I})
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I find this notation really confusing. Normally we'd have (key=>val) in dict, and this rearranges it to val in (dict=>key). There can be no good reason for that.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

good point. this was holdover from an earlier, more convoluted design iteration. this can be switched now in a future rev of the PR.

@vtjnash vtjnash force-pushed the jn/iocontext branch 3 times, most recently from 4ccd153 to 265670c Compare November 16, 2015 14:43
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 16, 2015

bump. i completed the implementation of this proposal over the weekend and would appreciate review.

@IainNZ
Copy link
Member

IainNZ commented Nov 17, 2015

I haven't reviewed the first few commits, but I've looked at some the rest, and it definitely looks like a good idea. Elimination of those showcompacts is very nice

@vtjnash vtjnash changed the title RFC: proposal for fixing show, print, & friends with IOContext proposal for fixing show, print, & friends with IOContext Dec 23, 2015
this is designed to be used as the basis for the IOContext type

also fix a bug in testing equality of a self-referential dict (with added test)
…d in a new IOContext lightweight wrapper type
…ting of a method signature

for example, this turns:
rand{T<:Union{Bool,Int16,Int32,Int64}}(::Type{T<:Union{Bool,Int16,Int32,Int64}}) at random.jl:38
into:
rand{T<:Union{Bool,Int16,Int32,Int64}}(::Type{T}) at random.jl:38

and it resolves ambiguous expressions such as:
foo{T,N}(::Array{T,N}, ::Vector, ::Array)
which used to be printed as:
foo{T,N}(::Array{T,N}, ::Array{T,1}, ::Array{T,N})
by printing it instead as:
foo{T,N}(::Array{T,N}, ::Array{T<:Any,1}, ::Array{T<:Any,N<:Any})
@vtjnash vtjnash changed the title proposal for fixing show, print, & friends with IOContext introduce IOContext and ImmutableDict to fix some of show, print, & friends Dec 24, 2015
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Dec 24, 2015

this now has tests and the actions items i had are closed. (this does not yet implement any of the changes discussed in #14052 as those were largely independent)

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Dec 25, 2015

counterpoint: it has near zero value without this PR (e.g. i wouldn't want to add it to base unless this PR also simultaneously got merged -- hence one PR)

@tkelman
Copy link
Contributor

tkelman commented Dec 25, 2015

Then how much value does it have within this PR? Is it really necessary?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Dec 25, 2015

it is an implementation detail for IOContext

vtjnash added a commit that referenced this pull request Dec 31, 2015
introduce IOContext and ImmutableDict to fix some of show, print, & friends
@vtjnash vtjnash merged commit 02aeb44 into master Dec 31, 2015
@vtjnash vtjnash deleted the jn/iocontext branch December 31, 2015 02:45
@timholy
Copy link
Sponsor Member

timholy commented Jan 2, 2016

This is very nice, thanks for doing this.

Looks like these with_output_lim got missed, though. (We could use some more dump tests, obviously.)

@timholy
Copy link
Sponsor Member

timholy commented Jan 2, 2016

See also #14533.

@JeffBezanson
Copy link
Sponsor Member

I would rename iosize. That name sounds like it might involve a size in bytes. It should clarify that it refers to the size of a terminal window, maybe terminalsize.

We also need to figure out a better way to deal with properties like limit_output. After all, we wanted to reduce the number of printing-related exports. We don't want a new export for every io property, nor do we want only some properties to have their own top-level functions. Though if I have to pick, the second one is worse since it makes the API less predictable.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2016

maybe displaysize, since it is expected to be primarily used with display?

if we don't want more exports, it's hard to get more concise than just using the definition everywhere:
limit_output(io::IO) = (get(io, :limit_output, false) === true)

i'm not opposed to that. the named function version was a bit of an experiment in transitioning, but i didn't find it to be any more usable

@tkelman
Copy link
Contributor

tkelman commented Jan 2, 2016

+1 for not exporting display-property-checker functions

@JeffBezanson
Copy link
Sponsor Member

displaysize sounds good.

vtjnash added a commit that referenced this pull request Jan 7, 2016
vtjnash added a commit that referenced this pull request Jan 7, 2016
for (key, value) in l
if value != get(r, key, secret_table_token)
for pair in l
if !in(pair, r, ==)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this change (together with the type constraint in the in above) makes custom Associatives that doesn't define their own == and return tuples in their iterator to raise method error in comparison. Is this intended? @vtjnash

This causes a test failure in PyCall. c.c. @stevengj

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

the type constraint is due to the expectation that an Associative only contains Pairs (e.g. see the error message you get if you look for something else: https://github.com/JuliaLang/julia/pull/13825/files#diff-d4736682004c1996e5d9b1c5c33c8c8eR28)

davidagold added a commit to JuliaStats/NullableArrays.jl that referenced this pull request Jan 16, 2016
Issue caused by changes to alignment and related method signatures in the following PR: JuliaLang/julia#13825
davidagold added a commit to JuliaStats/NullableArrays.jl that referenced this pull request Jan 17, 2016
Issue caused by changes to alignment and related method signatures in the
following PR: JuliaLang/julia#13825
davidagold added a commit to JuliaStats/NullableArrays.jl that referenced this pull request Jan 17, 2016
Issue caused by changes to alignment and related method signatures in the
following PR: JuliaLang/julia#13825
davidagold added a commit to JuliaStats/NullableArrays.jl that referenced this pull request Jan 17, 2016
Issue caused by changes to alignment and related method signatures in the
following PR: JuliaLang/julia#13825
davidagold added a commit to JuliaStats/NullableArrays.jl that referenced this pull request Jan 17, 2016
Issue caused by changes to alignment and related method signatures in the
following PR: JuliaLang/julia#13825
@nalimilan
Copy link
Member

@vtjnash The displaysize() method without any arguments is undocumented. Since it's based on ENV and doesn't seem react to changes in the terminal window size, what's it's purpose? How could we describe it in the docs?

@nalimilan
Copy link
Member

@vtjnash Bump? Could you explain the purpose of displaysize in a few words? I can't fix the docs myself without this information.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Apr 18, 2016

i generally considered it internal in that you shouldn't need to call it unless you are implementing a new display. it returns a generic size suitable for use by IO streams that couldn't compute the actual size

@nalimilan
Copy link
Member

Either it should be documented, or it should be another (unexported) function. The risk here is that people porting from the deprecated tty_size() will use displaysize() without any argument because it's similar. Which of the two solutions would you prefer?

@hayd
Copy link
Member

hayd commented Sep 18, 2016

Should ImmutableDict be exported/documented? It feels like it could be useful for defining a FrozenSet/FrozenDict in DataStructures.

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.

Should code_warntype integrate with the writemime system? Can we have print_color and println_color?
9 participants