-
Notifications
You must be signed in to change notification settings - Fork 368
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
Adding support for rich display of Markdown cells #2346
Changes from 18 commits
273bbb3
e72d033
a1d9619
57a3b58
531e089
ea571e6
c53598e
5df96a6
a391912
5f5f121
73a9010
3a85fbf
8928695
1a0fd3b
c1e38d4
be35852
7d4e5d0
2f5c2d3
c05e3ea
b90696d
606f3eb
174d632
5414031
17b6e1b
7a8c0de
c112ef4
8a04964
9fbe264
43a047c
8ff0ff0
e67308e
ab4209c
4993be3
feb0f5d
bbd5143
088c894
6a2ed0d
b3866e5
0f6fb38
1342b4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,11 @@ ourshow(io::IO, x::Symbol) = ourshow(io, string(x)) | |
ourshow(io::IO, x::Nothing; styled::Bool=false) = ourshow(io, "", styled=styled) | ||
ourshow(io::IO, x::SHOW_TABULAR_TYPES; styled::Bool=false) = | ||
ourshow(io, summary(x), styled=styled) | ||
function ourshow(io::IO, x::Markdown.MD) | ||
r = repr(x) | ||
len = min(length(r, 1, something(findfirst(==('\n'), r), lastindex(r)+1)-1), 32) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't abbreviate in other formats, so why do it for Markdown? Regarding newlines, I would just print them as We need a general mechanism to abbreviate too long cells, but this should be applied systematically disregarding the column type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would keep it here for the time being as In general maybe we will switch to PrettyTables.jl which actually handles this already. These changes will be possible to implement after 1.0 release if needed, as display changes are not considered to be breaking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, let's do that. Though do you think it would make sense to apply this 32-char limit to other types as well? I guess it can be wasteful when there are only a few columns and that more space is available, but it's hard to handle. How does PrettyTables handle it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But how wide terminal displays do you expect people have? PrettyTables.jl behaves like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, currently we can set the size of a column or its minimum size. However, I think it will be nice to have an option to set the maximum size also. In this case, we can set it to 32. Thus, columns that can be displayed with less characters will be, but columns larger than this will be cropped just like this: julia> pretty_table(df, alignment = :l, columns_width = [10,10])
┌────────────┬────────────┐
│ a │ b │
│ String │ String │
├────────────┼────────────┤
│ aaaaaaaaa… │ bbbbbbbbb… │
└────────────┴────────────┘ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! It is in julia> df = DataFrame(a="a"^5, b="b"^10, c="c"^15, d = "d"^20);
julia> pretty_table(df, alignment = :l, maximum_columns_width = 10)
┌────────┬────────────┬────────────┬────────────┐
│ a │ b │ c │ d │
│ String │ String │ String │ String │
├────────┼────────────┼────────────┼────────────┤
│ aaaaa │ bbbbbbbbbb │ ccccccccc… │ ddddddddd… │
└────────┴────────────┴────────────┴────────────┘ Off-topic: I think I just need to add 2 more features before starting to testing good configurations to print DataFrames! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, then maybe we should set a limit to 32 characters for all types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NicholasWMRitchie - would you be willing to make this change everywhere in this PR or we leave it for a follow up PR (I can do it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what's the conclusion on this? |
||
return print(io, len < length(r) - 1 ? first(r, len)*'…' : first(r, len)) | ||
end | ||
|
||
# AbstractChar: https://github.com/JuliaLang/julia/pull/34730 (1.5.0-DEV.261) | ||
# Irrational: https://github.com/JuliaLang/julia/pull/34741 (1.5.0-DEV.266) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean here by ", Markdown"? (we do not test shipping out data frame in Markdown anywhere - right?)