Convert the one and two argument show methods to MIME"text/plain" methods

[andreasnoack]

Strictly speaking, I consider this a bugfix. "Decorated" multiline output should be handled by a show method for MIME"text/plain" and not the two-argument show method. However, the show method here is very useful and probably used by many downstream packages, so it might cause a temporary ecosystem imbalance if this is released as a non-breaking release. We saw that when fixing the show method for StatsBase's CoefTable. Are there other changes waiting for a breaking release?

[bkamins]

Thank you for the PR. We do not plan to make breaking releases for this package. However, we can discuss if we can consider this a "bugfix" and do staged introduction of the changes.

For my understanding - what is exactly the bug that we fix with this change. (i.e. what is not working as expected with the current design)?

CC @nalimilan

[andreasnoack]

This is my understanding of https://docs.julialang.org/en/v1/manual/types/#man-custom-pretty-printing is that multi-line output should be delegated to the three-argument show method for MIME"text/plain" and also that the two argument method should ideally produce parseable output (which this PR doesn't resolve).

The immediate reason why I prepared this PR was vector output like the output below, which takes up too much space, is hard to separate the elements, and is a mess in text logs.

julia> fill(DataFrame(x = ones(5)), 5)
5-element Vector{DataFrame}:
 5×1 DataFrame
 Row │ x
     │ Float64
─────┼─────────
   1 │     1.0
   2 │     1.0
   3 │     1.0
   4 │     1.0
   5 │     1.0
 5×1 DataFrame
 Row │ x
     │ Float64
─────┼─────────
   1 │     1.0
   2 │     1.0
   3 │     1.0
   4 │     1.0
   5 │     1.0
 5×1 DataFrame
 Row │ x
     │ Float64
─────┼─────────
   1 │     1.0
   2 │     1.0
   3 │     1.0
   4 │     1.0
   5 │     1.0
 5×1 DataFrame
 Row │ x
     │ Float64
─────┼─────────
   1 │     1.0
   2 │     1.0
   3 │     1.0
   4 │     1.0
   5 │     1.0
 5×1 DataFrame
 Row │ x
     │ Float64
─────┼─────────
   1 │     1.0
   2 │     1.0
   3 │     1.0
   4 │     1.0
   5 │     1.0

With this PR, that would be

julia> fill(DataFrame(x = ones(5)), 5)
5-element Vector{DataFrame}:
 DataFrame(AbstractVector[[1.0, 1.0, 1.0, 1.0, 1.0]], DataFrames.Index(Dict(:x => 1), [:x]), nothing, nothing, true)
 DataFrame(AbstractVector[[1.0, 1.0, 1.0, 1.0, 1.0]], DataFrames.Index(Dict(:x => 1), [:x]), nothing, nothing, true)
 DataFrame(AbstractVector[[1.0, 1.0, 1.0, 1.0, 1.0]], DataFrames.Index(Dict(:x => 1), [:x]), nothing, nothing, true)
 DataFrame(AbstractVector[[1.0, 1.0, 1.0, 1.0, 1.0]], DataFrames.Index(Dict(:x => 1), [:x]), nothing, nothing, true)
 DataFrame(AbstractVector[[1.0, 1.0, 1.0, 1.0, 1.0]], DataFrames.Index(Dict(:x => 1), [:x]), nothing, nothing, true)

which can be improved, but is more structured and doesn't ruin text logs.

[bkamins]

Regarding the decision - I am OK to mark this PR as a bug fix.
However:

• @ronisbr - can you please chceck if this does not break anything on your end?
• @nalimilan - I assume you have experience with the CoefTable change and can judge how we should go about releasing this change

Thank you!

[ronisbr]

Hi!

Although I agree that the current way DataFrames print does not follow the convention in the Julia manual, I really think we must treat this as breaking. Anyone, me included, that customizes the output will start to see errors when updating the package. Furthermore, output customization will start to be more verbose.

The only gain, IMHO, is when you have a vector of DataFrames. However, it seems something very unusual.

Finally, if you decide to merge this PR, I highly recommend keeping the actual behavior if the user passes any keyword argument to show. In other words, print the compact version if and only if no keywords are present.

[andreasnoack]

I think I agree that, in its current form, this PR is probably too breaking to be considered just a bug fix. I do think it is unfortunate that the (very powerful and well written) show methods in this package don't follow the documentation for extending the show methods so eventually, it would be good to adjust them.

I could try to make this less breaking, but the feasibility of that depends on how people are actually using these custom show methods. If it only because people have been relying on show(::AbstractDataFrame; kwargs) then we could just keep that method for the time being. However, I'm a daily user of DataFrames and never called show explicitly on a DataFrame.

For CoefTable, the issue was that people used it as a part of their own custom show methods, which meant they were calling the the two-argument show method for CoefTable. If something similar is the case for DataFrames then it would cause a lot of breakage. I ended up searching through all packages with JuliaHub's code search to identify packages relying on CoefTable for their show methods and then either fixed the show method or suggested how to do so.

Alternatively, we can just keep this one open until there is an appetite for a major release. At that point, it might be a good idea to also convert all the keyword arguments to fields of the IOContext such that the show methods here fully following the intention of the display machinery.

[nalimilan]

I agree that it seems too breaking (for CoefTable there weren't any show methods taking keyword arguments and end users were unlikely to call show directly). I guess we could do what @ronisbr proposed above:

Finally, if you decide to merge this PR, I highly recommend keeping the actual behavior if the user passes any keyword argument to show. In other words, print the compact version if and only if no keywords are present.

Though it would be quite inconsistent so I'm not sure it would be worth it.

The issue isn't easy to resolve even for the next breaking release: requiring users to write show(df, MIME("text/plain"), ...) is a no-go IMO as this has to be convenient. It's kind of weird that it's harder in Julia to get the pretty-printed representation than the parseable representation. Maybe we should introduce a separate method, but in that case it should be shared with other packages.

[andreasnoack]

Do users really call show explicitly in interactive use cases? As mentioned above, I use DataFrames on daily basis and I've never called show expect in library code where I don't consider the extra verbosity an issue.

[nalimilan]

Hard to tell, but at least I would think that arguments like allrows=true are often useful.

[ronisbr]

Do users really call show explicitly in interactive use cases? As mentioned above, I use DataFrames on daily basis and I've never called show expect in library code where I don't consider the extra verbosity an issue.

We often receive some questions in Discourse asking how the output can be customized. That's was one of the reasons why @bkamins always asked me to add a section "Customizing DataFrames Output" to the manual. Hence, I think the number of people using it is not negligible. Now, with PrettyTables.jl v3 and the ability to add summary rows, I think it will be higher.

[bkamins]

In general my opinion is that users do ask for customized output in interactive sessions. Also in simple scripts calling show(df) is quite typical.