Skip to content

Only warn once about '...' contents in merge.data.table #7091

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.

Sign up for GitHub

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

Merged
MichaelChirico merged 11 commits into from
Jun 24, 2025
Merged

Only warn once about '...' contents in merge.data.table #7091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a8ff6e2
only warn once about '...' contents
MichaelChirico Jun 24, 2025
840c471
pluralization mess
MichaelChirico Jun 24, 2025
a0643df
fix old tests
MichaelChirico Jun 24, 2025
e33bb53
new tests
MichaelChirico Jun 24, 2025
a7379a3
break up double-ngettext
MichaelChirico Jun 24, 2025
d2ed302
missing ','
MichaelChirico Jun 24, 2025
185a783
fix vestige
MichaelChirico Jun 24, 2025
fe1eab0
also missing n_named
MichaelChirico Jun 24, 2025
b50f5d0
sprintf() to fill out; fix some tests
MichaelChirico Jun 24, 2025
514d963
missing 's'
MichaelChirico Jun 24, 2025
6913d5c
push logic into helper to tidy up merge body
MichaelChirico Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions R/merge.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,8 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
}

# warn about unused arguments #2587
if (length(list(...))) {
ell = as.list(substitute(list(...)))[-1L]
for (n in setdiff(names(ell), "")) warningf("Unknown argument '%s' has been passed.", n)
unnamed_n = length(ell) - sum(nzchar(names(ell)))
if (unnamed_n)
warningf("Passed %d unknown and unnamed arguments.", unnamed_n)
}
.maybe_warn_merge_dots(...)

# with i. prefix in v1.9.3, this goes away. Left here for now ...
## sidestep the auto-increment column number feature-leading-to-bug by
## ensuring no names end in ".1", see unit test
Expand Down Expand Up @@ -125,3 +120,28 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
setattr(dt, "class", class_x)
dt
}

.maybe_warn_merge_dots <- function(...) {
# TODO(R >= 3.5.0): use ...length()
n_dots <- length(dots <- list(...))
if (!n_dots) return(invisible())

nm <- names(dots)
if (is.null(nm)) {
warningf(ngettext(n_dots, "merge.data.table() received %d unnamed argument in '...' which will be ignored.",
"merge.data.table() received %d unnamed arguments in '...' which will be ignored."),
n_dots)
} else {
named_idx = nzchar(nm)
if (all(named_idx)) {
warningf(ngettext(n_dots, "merge.data.table() received %d unknown keyword argument which will be ignored: %s",
"merge.data.table() received %d unknown keyword arguments which will be ignored: %s"),
n_dots, brackify(nm))
} else {
n_named <- sum(named_idx)
unnamed_clause <- sprintf(ngettext(n_dots - n_named, "%d unnamed argument in '...'", "%d unnamed arguments in '...'"), n_dots - n_named)
named_clause <- sprintf(ngettext(n_named, "%d unknown keyword argument", "%d unknown keyword arguments"), n_named)
warningf("merge.data.table() received %s and %s, all of which will be ignored: %s", unnamed_clause, named_clause, brackify(nm[named_idx]))
}
}
}
28 changes: 20 additions & 8 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17981,14 +17981,26 @@ test(2229.6, fread(testDir("multi-file.zip")), error="Compressed files containin
x = data.frame(k1 = c(NA,NA,3,4,5), k2 = c(1,NA,NA,4,5), data = 1:5)
y = data.frame(k1 = c(NA,2,NA,4,5), k2 = c(NA,NA,3,4,5), data = 1:5)
DT = as.data.table(x)
test(2230.1, setDF(merge(DT, y, by="k2", incomparables=NA)), merge(x, y, by="k2", incomparables=NA))
test(2230.2, setDF(merge(DT, y, by="k2", incomparables=c(NA,4))), merge(x, y, by="k2", incomparables=c(NA,4)))
test(2230.3, setDF(merge(DT, y, by="k2", incomparables=c(4,5))), merge(x, y, by="k2", incomparables=c(4,5)))
test(2230.4, setDF(merge(DT, y, by="k2", incomparables=c(1, NA, 4, 5))), merge(x, y, by="k2", incomparables=c(1,NA,4,5)))
test(2230.5, setDF(merge(DT, y, by="k2", incomparables=c(NA, 3, 4, 5))), merge(x, y, by="k2", incomparables=c(NA,3,4,5)))
test(2230.6, merge(DT, y, by="k2", unk=1), merge(DT, y, by="k2"), warning="Unknown argument 'unk' has been passed.")
test(2230.7, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, 1L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "Passed 1 unknown and unnamed arguments."))
test(2230.01, setDF(merge(DT, y, by="k2", incomparables=NA)), merge(x, y, by="k2", incomparables=NA))
test(2230.02, setDF(merge(DT, y, by="k2", incomparables=c(NA,4))), merge(x, y, by="k2", incomparables=c(NA,4)))
test(2230.03, setDF(merge(DT, y, by="k2", incomparables=c(4,5))), merge(x, y, by="k2", incomparables=c(4,5)))
test(2230.04, setDF(merge(DT, y, by="k2", incomparables=c(1, NA, 4, 5))), merge(x, y, by="k2", incomparables=c(1,NA,4,5)))
test(2230.05, setDF(merge(DT, y, by="k2", incomparables=c(NA, 3, 4, 5))), merge(x, y, by="k2", incomparables=c(NA,3,4,5)))
test(2230.06, merge(DT, y, by="k2", unk=1), merge(DT, y, by="k2"), warning="1 unknown keyword argument which will be ignored: [unk]")
test(2230.07, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, 1L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "1 unnamed argument in '...' which will be ignored."))
test(2230.08, merge(DT, y, by="k2", unk1=1, unk2=2), merge(DT, y, by="k2"), warning="2 unknown keyword arguments which will be ignored: [unk1, unk2]")
test(2230.09, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, 1L, 2L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "2 unnamed arguments in '...' which will be ignored."))
test(2230.10, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, unk=1L, 2L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "1 unnamed argument .*1 unknown keyword argument,.*\\[unk\\]"))
test(2230.11, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, unk1=1L, unk2=2L, 3L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "1 unnamed argument .*2 unknown keyword arguments.*\\[unk1, unk2\\]"))
test(2230.12, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, unk=1L, 2L, 3L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "2 unnamed arguments.*1 unknown keyword argument,.*\\[unk\\]"))
test(2230.13, merge(DT, y, by="k2", NULL, NULL, FALSE, FALSE, FALSE, TRUE, c(".x", ".y"), TRUE, getOption("datatable.allow.cartesian"), NULL, unk1=1L, unk2=2L, 3L, 4L),
merge(DT, y, by="k2"), warning=c("Supplied both `by` and `by.x`/`by.y`. `by` argument will be ignored.", "2 unnamed arguments.*2 unknown keyword arguments.*\\[unk1, unk2\\]"))


# weighted.mean GForce optimized, #3977
old = options(datatable.optimize=1L)
Expand Down
Loading