Skip to content
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

Ignore names when an rlang::zap() is supplied as .name_spec #1091

Merged
merged 3 commits into from May 6, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions NEWS.md
Expand Up @@ -190,6 +190,11 @@ The following errors are caused by breaking changes.

* `new_data_frame()` infers size from row names when `n = NULL` (#894).

* `vec_c()` now accepts `rlang::zap()` as `.name_spec` input. The
returned vector is then always unnamed, and the names do not cause
errors when they can't be combined. They are still used to create
more informative messages when the inputs have incompatible types (#232).


## Classes

Expand Down
5 changes: 4 additions & 1 deletion R/c.R
Expand Up @@ -7,7 +7,10 @@
#' * `vec_ptype(vec_c(x, y)) == vec_ptype_common(x, y)`.
#'
#' @param ... Vectors to coerce.
#' @param .name_repair How to repair names, see `repair` options in [vec_as_names()].
#' @param .name_repair How to repair names, see `repair` options in
#' [vec_as_names()]. Can also be [rlang::zap()] to ignore names
#' during concatenation. The names are still used to give
#' informative error messages, e.g. with coercion errors.
#' @return A vector with class given by `.ptype`, and length equal to the
#' sum of the `vec_size()` of the contents of `...`.
#'
Expand Down
5 changes: 4 additions & 1 deletion man/vec_c.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion man/vec_chop.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions src/c.c
Expand Up @@ -63,7 +63,8 @@ SEXP vec_c(SEXP xs,
int* idx_ptr = INTEGER(idx);

SEXP xs_names = PROTECT(r_names(xs));
bool has_names = xs_names != R_NilValue || list_has_inner_vec_names(xs, n);
bool assign_names = !Rf_inherits(name_spec, "rlang_zap");
bool has_names = assign_names && (xs_names != R_NilValue || list_has_inner_vec_names(xs, n));
has_names = has_names && !is_data_frame(ptype);

PROTECT_INDEX out_names_pi;
Expand All @@ -74,7 +75,7 @@ SEXP vec_c(SEXP xs,
R_len_t counter = 0;

const struct vec_assign_opts c_assign_opts = {
.assign_names = true
.assign_names = assign_names
};

for (R_len_t i = 0; i < n; ++i) {
Expand All @@ -83,7 +84,6 @@ SEXP vec_c(SEXP xs,
continue;
}

// TODO
SEXP x = VECTOR_ELT(xs, i);
SEXP elt = PROTECT(vec_cast(x, ptype, args_empty, args_empty));

Expand Down Expand Up @@ -115,8 +115,12 @@ SEXP vec_c(SEXP xs,
if (has_names) {
out_names = PROTECT(vec_as_names(out_names, name_repair));
out = vec_set_names(out, out_names);
REPROTECT(out, out_pi);
UNPROTECT(1);
} else if (!assign_names) {
// FIXME: `vec_ptype2()` doesn't consistently zaps names, so `out`
// might have been initialised with names. This branch can be
// removed once #1020 is resolved.
out = vec_set_names(out, R_NilValue);
}

UNPROTECT(7);
Expand Down
4 changes: 4 additions & 0 deletions src/names.c
Expand Up @@ -515,6 +515,10 @@ static SEXP glue_as_name_spec(SEXP spec);

// [[ include("utils.h") ]]
SEXP apply_name_spec(SEXP name_spec, SEXP outer, SEXP inner, R_len_t n) {
if (Rf_inherits(name_spec, "rlang_zap")) {
return R_NilValue;
}

if (outer == R_NilValue) {
return inner;
}
Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/error/test-c.txt
Expand Up @@ -34,3 +34,10 @@ See <https://vctrs.r-lib.org/articles/s3-vector.html>.
> with_c_foobar(vec_c(foobar(1), foobar(2), .ptype = ""))
Error: Can't convert <vctrs_foobar> to <character>.


can ignore names by providing a `zap()` name-spec (#232)
========================================================

> vec_c(a = c(b = letters), b = 1, .name_spec = zap())
Error: Can't combine `a` <character> and `b` <double>.

7 changes: 7 additions & 0 deletions tests/testthat/error/test-unchop.txt
Expand Up @@ -72,3 +72,10 @@ Error: Must subset elements with a valid subscript vector.
x Subscript has the wrong type `vctrs_foobar`.
i It must be numeric.


can ignore names in `vec_unchop()` by providing a `zap()` name-spec (#232)
==========================================================================

> vec_unchop(list(a = c(b = letters), b = 3L), name_spec = zap())
Error: Can't combine `a` <character> and `b` <integer>.

14 changes: 14 additions & 0 deletions tests/testthat/test-c.R
Expand Up @@ -324,6 +324,17 @@ test_that("vec_implements_ptype2() and vec_c() fallback are compatible with old
expect_identical(vec_c(bar), bar)
})

test_that("can ignore names in `vec_c()` by providing a `zap()` name-spec (#232)", {
expect_error(vec_c(a = c(b = 1:2)))
expect_identical(vec_c(a = c(b = 1:2), b = 3L, .name_spec = zap()), 1:3)
verify_errors({
expect_error(
vec_c(a = c(b = letters), b = 1, .name_spec = zap()),
class = "vctrs_error_incompatible_type"
)
})
})

test_that("vec_c() has informative error messages", {
verify_output(test_path("error", "test-c.txt"), {
"# vec_c() fails with complex foreign S3 classes"
Expand All @@ -339,5 +350,8 @@ test_that("vec_c() has informative error messages", {
"# vec_c() fallback doesn't support `name_spec` or `ptype`"
with_c_foobar(vec_c(foobar(1), foobar(2), .name_spec = "{outer}_{inner}"))
with_c_foobar(vec_c(foobar(1), foobar(2), .ptype = ""))

"# can ignore names by providing a `zap()` name-spec (#232)"
vec_c(a = c(b = letters), b = 1, .name_spec = zap())
})
})
14 changes: 14 additions & 0 deletions tests/testthat/test-slice-chop.R
Expand Up @@ -687,6 +687,17 @@ test_that("vec_unchop() does not support non-numeric S3 indices", {
})
})

test_that("can ignore names in `vec_unchop()` by providing a `zap()` name-spec (#232)", {
expect_error(vec_unchop(list(a = c(b = 1:2))))
expect_identical(vec_unchop(list(a = c(b = 1:2), b = 3L), name_spec = zap()), 1:3)
verify_errors({
expect_error(
vec_unchop(list(a = c(b = letters), b = 3L), name_spec = zap()),
class = "vctrs_error_incompatible_type"
)
})
})

test_that("vec_unchop() has informative error messages", {
verify_output(test_path("error", "test-unchop.txt"), {
"# vec_unchop() errors on unsupported location values"
Expand Down Expand Up @@ -717,5 +728,8 @@ test_that("vec_unchop() has informative error messages", {
"# vec_unchop() does not support non-numeric S3 indices"
vec_unchop(list(1), list(factor("x")))
vec_unchop(list(1), list(foobar(1L)))

"# can ignore names in `vec_unchop()` by providing a `zap()` name-spec (#232)"
vec_unchop(list(a = c(b = letters), b = 3L), name_spec = zap())
})
})