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

Deprecate showcompact() #26080

Merged
merged 2 commits into from
Mar 9, 2018
Merged

Deprecate showcompact() #26080

merged 2 commits into from
Mar 9, 2018

Conversation

nalimilan
Copy link
Member

There is no obvious reason to provide a special function for the :compact IOContext property
but not for other properties. showcompact used to be useful to print a single-line representation of an array at the REPL, but now show does the same thing. So it should only be needed for collections to print their elements when implementing show, and for tests (most of the changes here).

Also improve a few docstrings.

@nalimilan nalimilan added the display and printing Aesthetics and correctness of printed representations of objects. label Feb 16, 2018
NEWS.md Outdated

* `showcompact(io, x...)` has been deprecated in favor of
`show(IOContext(io, :compact => true), x...)` ([#26080]).
Use `sprint(showcompact, x...)` instead of `sprint(show, x..., context=:compact => true)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's the wrong way around, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I had originally written "is deprecated in favor of", but I changed it because it's not really a separate deprecation, mostly an advice. Will fix in the next round.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@JeffBezanson
Copy link
Member

The downside is this has to allocate a new IOContext on every call, but I guess that's life.


Call the given function with an I/O stream and the supplied extra arguments.
Everything written to this I/O stream is returned as a string.
`context` can be either an [`IOContext`](@ref) whose properties will be used,
or a `Pair` specifying a property and its value. `sizehint` suggests the capacity
Copy link
Member

Choose a reason for hiding this comment

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

Specifying a Pair does happen to work, but only the usage case of passing an IO was what I had really originally intended (and found to be common).

I think that we maybe instead should suggest the usage of anonymous functions as a general way of passing a configuration set of arbitrarily many new properties:

showcompact = (io, v) -> show(IOContext(io, :compact => true), v)
sprint(showcompact, 66.66666)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I assumed it was intended since the signature doesn't use ::IOContext.

I actually started with the anonymous function approach, but it's quite verbose and much less convenient than passing a pair for common cases (have a look at the diff to get and idea). So I'd say we'd better continue to support passing a pair.

Copy link
Member

Choose a reason for hiding this comment

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

I don't intend to prevent passing a Pair, but it simply doesn't generalize as well. Although I suppose it also quickly becomes easier just to write it out:

buf = IOBuffer(sizehint = sizehint)
io = IOContext(buf, :compact => true)
show(io, 66.66666)
return String(take!(buf))

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was that passing a single pair is the most common case, and for other situations it's not too hard to create a full IOContext.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's very nice to be able to pass a single pair.

@ararslan ararslan added the deprecation This change introduces or involves a deprecation label Feb 16, 2018
@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Feb 28, 2018
@ararslan
Copy link
Member

ararslan commented Mar 1, 2018

If people are concerned about the IOContext part, perhaps we could add a compact keyword argument to printstyled?

@JeffBezanson
Copy link
Member

Triage is in favor of removing showcompact, and improving the documentation about how to implement it correctly via show.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Mar 8, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Mar 8, 2018
There is no obvious reason to provide a special function for the :compact IOContext property
but not for other properties. showcompact() used to be useful to print a single-line
representation of an array at the REPL, but now show() does the same thing. So it should only
be needed for collections to print their elements when implementing show(), and for tests
(most of the changes here).

Also improve a few docstrings.
@nalimilan
Copy link
Member Author

I've rebased and added a short presentation of IOContext and :compact in the "Custom pretty-printing" section of the manual. Let me know whether it's OK.

@JeffBezanson
Copy link
Member

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants