-
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
Closed
Closed
Changes from 14 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
273bbb3
Adding support for rich display of Markdown cells
NicholasWMRitchie e72d033
Fixing Markdown as "text/csv" and "text/plain"
NicholasWMRitchie a1d9619
Fix test error in Markdown to HTML in 32-bit systems
NicholasWMRitchie 57a3b58
Fixing show and adding multi-codepoint tests
NicholasWMRitchie 531e089
32 it is...
NicholasWMRitchie ea571e6
Use chomp to remove trailing '\n
NicholasWMRitchie c53598e
Fixing conflicts
NicholasWMRitchie 5df96a6
Merge branch 'master' into master
bkamins a391912
Don't match on AbstractVector type parameter due to compiler crash (#…
quinnj 5f5f121
Pulling df[i,j] out and optimizing ourshow.
NicholasWMRitchie 73a9010
Merge branch 'master' of https://github.com/NicholasWMRitchie/DataFra…
NicholasWMRitchie 3a85fbf
Formatting improvements
NicholasWMRitchie 8928695
Additional formatting improvements
NicholasWMRitchie 1a0fd3b
Update CONTRIBUTING.md
bkamins c1e38d4
Update CONTRIBUTING.md
bkamins be35852
Fixing triple-quoted strings in 1.0.X
NicholasWMRitchie 7d4e5d0
Strip space in test case
NicholasWMRitchie 2f5c2d3
Using @nalimilan suggestion
NicholasWMRitchie c05e3ea
Merge branch 'master' of https://github.com/NicholasWMRitchie/DataFra…
NicholasWMRitchie b90696d
Improving LaTeX output appearance, NEWS.md entry
NicholasWMRitchie 606f3eb
Moved NEWS.md item to "Other relevant changes" section.
NicholasWMRitchie 174d632
Correct NEWS.md item on rich display support
NicholasWMRitchie 5414031
Adding support for rich display of Markdown cells
NicholasWMRitchie 17b6e1b
Fixing Markdown as "text/csv" and "text/plain"
NicholasWMRitchie 7a8c0de
Fix test error in Markdown to HTML in 32-bit systems
NicholasWMRitchie c112ef4
Fixing show and adding multi-codepoint tests
NicholasWMRitchie 8a04964
32 it is...
NicholasWMRitchie 9fbe264
Use chomp to remove trailing '\n
NicholasWMRitchie 43a047c
Fixing conflicts
NicholasWMRitchie 8ff0ff0
Pulling df[i,j] out and optimizing ourshow.
NicholasWMRitchie e67308e
Formatting improvements
NicholasWMRitchie ab4209c
Additional formatting improvements
NicholasWMRitchie 4993be3
Fixing triple-quoted strings in 1.0.X
NicholasWMRitchie feb0f5d
Strip space in test case
NicholasWMRitchie bbd5143
Using @nalimilan suggestion
NicholasWMRitchie 088c894
Improving LaTeX output appearance, NEWS.md entry
NicholasWMRitchie 6a2ed0d
Moved NEWS.md item to "Other relevant changes" section.
NicholasWMRitchie b3866e5
Correct NEWS.md item on rich display support
NicholasWMRitchie 0f6fb38
Merge remote-tracking branch 'origin/master'
NicholasWMRitchie 1342b4b
Removing duplicate using lines
NicholasWMRitchie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We don't abbreviate in other formats, so why do it for Markdown? Regarding newlines, I would just print them as
\n
just like we do for strings.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 comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep it here for the time being as
md"
is likely to be overlong. Then maybe just add TODO to update it when we have a general mechanism.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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
But how wide terminal displays do you expect people have?
I have 210 characters width in my terminal, but having a column wider than 32 chars makes it very hard to follow what is in the column anyway. So I would say the rule - no column wider than 32 characters makes sense. If someone wants to check such a wide column in detail in REPL probably extracting it to a vector and investigating is the way to go anyway.
PrettyTables.jl behaves like this:
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.
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:
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.
nice
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.
Done! It is in
master
: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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
So what's the conclusion on this?