Fixed quotes showing in tables() output in dev. #2565
Conversation
|
starting to travel, will try and pull and check offline while so. review
next wk
…On Jan 12, 2018 10:44 PM, "Matt Dowle" ***@***.***> wrote:
@mattdowle <https://github.com/mattdowle> requested your review on:
Rdatatable/data.table#2565
<#2565> Fixed quotes showing
in tables() output in dev..
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2565 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHQQdW4SMPKm3n-KViyUDY9gCHTFdk2wks5tKCaUgaJpZM4RdFqf>
.
|
Codecov Report
@@ Coverage Diff @@
## master #2565 +/- ##
==========================================
- Coverage 91.39% 91.39% -0.01%
==========================================
Files 63 63
Lines 12101 12097 -4
==========================================
- Hits 11060 11056 -4
Misses 1041 1041
Continue to review full report at Codecov.
|
R/tables.R
Outdated
| if (mb) cat("Total: ", prettyNum(as.character(total), big.mark=","), "MB\n", sep="") | ||
| # prettier printing on console | ||
| tt = copy(info) | ||
| tt[ , NROW := format(sprintf("%4s", prettyNum(NROW, big.mark=",")), justify="right")] # %4s is for minimum width |
There was a problem hiding this comment.
I think the width argument of format.default can be used instead of calling sprintf:
format(prettyNum(NROW, big.mark=","), width = 4L, justify="right")
little test I ran to convince myself:
all(replicate(1000, {
ns = prettyNum(sample(20000, 10), big.mark = ',')
wid = format(sprintf('%4s', ns), justify = 'right')
spr = format(ns, width = 4L, justify = 'right')
identical(wid, spr)
}))
# [1] TRUE
There was a problem hiding this comment.
Anyway we use the format(prettyNum(...)) enough times here to warrant a function (commaNum? print_num?). Maybe defined within the scope of tables? Or something in utils.R? (just greped for sprintf, only used in this capacity here within tables)
R/tables.R
Outdated
| tt[ , NCOL := format(sprintf("%4s", prettyNum(NCOL, big.mark=",")), justify="right")] | ||
| if (mb) tt[ , MB := format(sprintf("%2s", prettyNum(MB, big.mark=",")), justify="right")] | ||
| print(tt, class=FALSE, nrow=Inf) | ||
| if (mb) cat("Total: ", prettyNum(as.character(sum(info$MB)), big.mark=","), "MB\n", sep="") |
There was a problem hiding this comment.
why as.character here? prettyNum should handle the type conversion?
There was a problem hiding this comment.
i think it's good to go. agree with move to use print.data.table facilities -- what's #1523 otherwise?
|
Thanks @MichaelChirico. Good spots and changes. |
In master before this PR I was seeing quotes and a NULL in the output of
tables()as follows.Seems to have arose in PR #1804 for issue #1648.
This PR simplifies
tables()by leaving more of the formatting to standardprint(DT)rather than creating a character matrix (something I did originally and was always a bit messy.)Please check @MichaelChirico -- thanks.