-
Notifications
You must be signed in to change notification settings - Fork 991
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
Print Option: Print columns that fit in single console without wrapping #4074
Conversation
Included the option onLoad and the docs
Codecov Report
@@ Coverage Diff @@
## master #4074 +/- ##
=========================================
+ Coverage 99.4% 99.4% +<.01%
=========================================
Files 72 72
Lines 13675 13710 +35
=========================================
+ Hits 13594 13629 +35
Misses 81 81
Continue to review full report at Codecov.
|
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 about some unit tests?
Added unit tests, fixed an issue that appeared with one row data tables when Should I add info to Also, regarding the classes, I was thinking we could add the classes to the "Variables not shown" message when |
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.
@MichaelChirico could you take a look? you are more into printing options features.
Added class to `trunc.cols` message
The failed test looks like it just needs to be re-run (it looks like a LaTeX error). I added the classes to the message about not printed columns when |
@TysonStanley please see the inline comments I added, still a few things to handle. Thanks! |
@MichaelChirico I’m not seeing the inline comments showing up. Am I missing it? Also, I like the updated |
@TysonStanley do you see this and several others on the https://github.com/Rdatatable/data.table/pull/4074/files |
@MichaelChirico no, I looked there and couldn’t see any... All I see are the changes to the files (no comments at all). |
R/print.data.table.R
Outdated
names = sapply(names, nchar_width) | ||
dt_widths = ifelse(widths > names, widths, names) | ||
rownum_width = if (row.names) nchar_width(nrow(x)) else 0 | ||
cumsum(dt_widths + 1) + rownum_width + 2 |
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.
this will print columns 1, 2, ..., n
where n
is the last column before crossing options('width')
right?
pandas
is doing a bracketing approach (columns (1, N)
, (2, N-1)
, ..., (n, N-n)
) where the (n+1, N-n-1)
pair would break the options('width')
threshold.
I'm not married to one way or the other, just worth considering which we think is the best fit. Are users likely to prefer seeing the first few columns, or seeing the first + last few? I'm not sure
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.
Yes, you are correct. My personal preference is the way I've done it but that, by no means, is a good reason. What do you suggest? Would having another option be the fix here?
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 think we should pick one and stick with it rather than add more options. We can play around with it in dev for a while & maybe change later.
Just some more thoughts: I guess most common is to have the first few columns with keys/"ID"-style rows. Then have "data" in the "rightmost" columns. So the tradeoff is -- showing all the key columns, vs showing some keys, some data. Still don't have a feel yet for which is more helpful to see in a preview.
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.
That makes sense. I like the idea of differentiating between the keys and "data" in the preview. Definitely could get to some weird edge cases with this, depending on the console width and the number of keys...
@TysonStanley aaahhh sorry. I forgot to hit submit on my review. Hence the |
The "only" is in the wrong place in that commit message. It should say: "prints only the message when console too small to fit the first column". E.g.,
|
This one adds the brackets to the tests. I like how those brackets look. |
renamed tests so they are in order
@MichaelChirico Alright, checks look good so I won't make any other changes unless you ask me to. |
Looks good to me! Thanks for your diligence!! |
Excellent. @TysonStanley I've invited you to be project member. You'll need to accept the invitation displayed in your GitHub profile or GitHub repositories page. Then you can create branches directly in the main project. Welcome! |
…moved news item up, bumped test number
@TysonStanley There should be better validation of argument values provided, for example following error message is very confusing.
|
Just made a PR for this #4766 that adds a quick check for a logical |
Closes #1497
This PR addresses @MichaelChirico's number 11 and, somewhat, number 16 on #1523 by providing a
trunc.cols
argument (with corresponding defaultoption("datatable.print.trunc.cols")
). For example:Some possible ideas for adjustments:
data.table
usual of showing the first and last columns.