Fix: Ensure column widths are data-driven when col.names = "none" in print.data.table #6916
Conversation
| @@ -139,6 +139,8 @@ print.data.table = function(x, topn=getOption("datatable.print.topn"), | |||
| print_default(toprint) | |||
| return(invisible(x)) | |||
| } | |||
| if (col.names == "none") | |||
There was a problem hiding this comment.
did you try making this change on lines 94-96 above instead?
There was a problem hiding this comment.
sorry i forgot to include ,i will try it now
There was a problem hiding this comment.
@MichaelChirico I tried implementing it
However, when I tried to integrate this with the existing column name clearing logic as you suggested
Tests 2125.06 and 2125.07 fail, which involve truncation messages when col.names="none".
The issue appears to be related to how the truncation message logic works in trunc_cols_message(). When column names are cleared early in the combined condition, the truncation logic can no longer access the original column names, resulting in empty brackets in messages:
- Expected: 1 variable not shown: [d]
- Observed: 1 variable not shown: []
With the standalone implementation, the column names are likely cleared after the truncation message logic has already determined which columns to display, maintaining the correct behavior.
There was a problem hiding this comment.
We could maybe store the removed column names temporarily and then make the change?
TBH if I used col.names == "none" I would not expect a x variable(s) not shown: [cols] printout, as I didn't want to see column names in the first place
There was a problem hiding this comment.
@joshhwuu Thanks for the suggestion! Looking at the test outputs, I agree with your point about user expectations - if someone uses col.names="none", they're explicitly indicating they don't want to see column names.
Based on these test results and your feedback, I think the simplest approach would be to modify the truncation message logic to show a simpler message (without column names) when col.names="none" is used:
if (col.names == "none") {
# Simpler message without column names
catf("%d variable%s not shown\n", n, if(n==1L) "" else "s", domain=NA)
} else {
# Original message with column names
# ... existing code ...
}
This approach:
- Maintains consistency with user expectations
- Avoids the complexity of storing/restoring column names temporarily
- Keeps the consolidated condition as suggested by @MichaelChirico
Would this work for everyone? I can implement this change alongside the fix for the column width issue.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6916 +/- ##
=======================================
Coverage 98.59% 98.59%
=======================================
Files 79 79
Lines 14685 14687 +2
=======================================
+ Hits 14479 14481 +2
Misses 206 206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @MichaelChirico , |
|
hi |
|
hi @joshhwuu , |
| test(2315.3, { | ||
| dt <- data.table(a=NA_integer_, b=NaN) | ||
| print(dt, col.names="none") | ||
| }, output="1: NA NaN\n") |
There was a problem hiding this comment.
Just a couple of small suggestions to align with the style used in inst/tests/tests.Rraw:
- Initialize data.table outside the test() call.
- No need for {}, since each test() contains only a single expression.
- Add a comment above the test block like:
# Tests for print() output with fixed index header (PR #6806)
This PR resolves #6882 by ensuring that when col.names = "none" is specified in print.data.table, the column widths are determined solely by the data content, not by the (hidden) column names.
What was changed
Added the following logic to print.data.table:
This is placed right after toprint is constructed, but before any formatting or width calculations.
This ensures that all downstream formatting and width calculations use empty column names, so column widths are always based on the data, fully resolving the issue where hidden long column names could cause excessive column width.
@MichaelChirico have a review of this when you have time
this is the change i was suggesting should i change it
or it works fine let me know
thank you.