Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7674 +/- ##
=======================================
Coverage 98.85% 98.85%
=======================================
Files 87 87
Lines 17128 17146 +18
=======================================
+ Hits 16932 16950 +18
Misses 196 196 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No obvious timing issues in HEAD=issue4044 Generated via commit 67cc9ef Download link for the artifact containing the test results: ↓ atime-results.zip
|
| test(2367.6, fread(file(f)), data.table(), warning="Connection has size 0.") | ||
| unlink(f) | ||
|
|
||
| #4044 |
There was a problem hiding this comment.
please dont only write the issue number, but what issue is fixed. this provides information without needing to search for the issue on github.
The tests seem ok but an example of DT = data.table(a=1:2, b=c('x', 'y')) would have been easier to grasp (and also less typing/code)
|
|
||
| process_name_policy = function(names_vec) { | ||
| policy = getOption("datatable.unique.names") | ||
| if (is.null(policy) || policy == "off") return(names_vec) |
There was a problem hiding this comment.
no ultra happy with this double option for "off". I would either eliminate NULL or "off" (leaning towards eliminating NULL though)
| \item{\code{datatable.enlist}}{Experimental feature. Default is \code{NULL}. If set to a function | ||
| (e.g., \code{list}), the \code{j} expression can return a \code{list}, which will then | ||
| be "enlisted" into columns in the result.} | ||
| \item{\code{datatable.unique.names}}{A character string, default \code{NULL} (same as \code{"off"}). |
There was a problem hiding this comment.
we should also document the other options, e.g. warn, error etc.
| msg = paste0("Duplicate column names created: ", brackify(dups), ". This may cause ambiguity.") | ||
|
|
||
| switch(policy, | ||
| warn = warningf(msg), |
There was a problem hiding this comment.
see comment above from previous review. this will trip with
DT = data.table("a%d" = 1, b = 2)
options(datatable.unique.names = "warn")
setnames(DT, "b", "a%d")
# Error in sprintf(gettext(fmt, domain = domain, trim = trim), ...) :
# too few argumentsHence, you need to pass format strings like warningf("%s", msg)
|
|
||
| 5. `tables()` can now optionally report `data.table` objects stored one level deep inside list objects when `depth=1L`, [#2606](https://github.com/Rdatatable/data.table/issues/2606). Thanks @MichaelChirico for the report and @manmita for the PR | ||
|
|
||
| 6. `setnames()` now supports a global option `datatable.unique.names` to control the creation of duplicate column names. Users can choose between `"off"` (default), `"warn"`, `"error"`, or `"rename"`. This addresses long-standing ambiguity issues when duplicate names were created silently, [#4044](https://github.com/Rdatatable/data.table/issues/4044). Thanks to @venom1204 for the PR. |
There was a problem hiding this comment.
I dont think we need this part since its superfluous "This addresses long-standing ambiguity issues when duplicate names were created silently"
| table_name, brackify(duplicate_names), domain=NA) | ||
| } | ||
|
|
||
| process_name_policy = function(names_vec) { |
There was a problem hiding this comment.
Also since we only use it with setnames it should probably live inside data.table.R in front of setnames
venom1204
changed the title
2 participants
Closes #4044
Hi @ben-schwen,
Apologies for opening a new PR—there was an issue with the previous one. I’ve incorporated the requested changes, while keeping the rest unchanged.
Kindly review it when you have the time. Also, sorry for the delay; I was occupied with other commitments.
Thank you!