Fix corner case in new_data_frame(n = NULL) #894

krlmlr · 2020-03-08T06:40:53Z

We should allow omitting n if row.names attribute is given.

Current behavior:

rlang::exec(
  vctrs::new_data_frame,
  list(),
  n = NULL, !!!attributes(data.frame(a = 1)[0])
)
#> Error: `n` and `row.names` must be consistent.

^{Created on 2020-03-08 by the reprex package (v0.3.0)}

With this patch:

rlang::exec(
  vctrs::new_data_frame,
  list(),
  n = NULL, !!!attributes(data.frame(a = 1)[0])
)
#> data frame with 0 columns and 1 row

^{Created on 2020-03-08 by the reprex package (v0.3.0)}

lionel- · 2020-03-09T08:38:20Z

src/type-data-frame.c

@@ -87,7 +87,9 @@ SEXP vctrs_new_data_frame(SEXP args) {
    if (tag == R_RowNamesSymbol) {
      // "row.names" is checked for consistency with n (if provided)
      if (size != rownames_size(CAR(node))) {
-        Rf_errorcall(R_NilValue, "`n` and `row.names` must be consistent.");
+        if (n != R_NilValue) {


Can you merge this if with the nesting if please.

lionel- · 2020-03-09T08:39:21Z

tests/testthat/test-type-data-frame.R

@@ -281,9 +281,10 @@ test_that("attributes with special names are merged", {
    "rowname"
  )

-  expect_error(
+  # Can omit n if row.names attribute is given


n is not omitted in this test. Should probably be its own test_that() block.

lionel- · 2020-03-09T10:38:05Z

Thanks!

Fix corner case in new_data_frame(n = NULL)

cb0c0dd

lionel- reviewed Mar 9, 2020

View reviewed changes

krlmlr added 2 commits March 9, 2020 10:25

Simplify

7cd27b2

Tweak tests

3219a7c

lionel- merged commit 0803c1b into r-lib:master Mar 9, 2020

Fix corner case in new_data_frame(n = NULL) #894

Fix corner case in new_data_frame(n = NULL) #894

krlmlr commented Mar 8, 2020 •

edited

lionel- Mar 9, 2020

lionel- Mar 9, 2020

lionel- commented Mar 9, 2020

Fix corner case in new_data_frame(n = NULL) #894

Fix corner case in new_data_frame(n = NULL) #894

Conversation

krlmlr commented Mar 8, 2020 • edited

lionel- Mar 9, 2020

Choose a reason for hiding this comment

lionel- Mar 9, 2020

Choose a reason for hiding this comment

lionel- commented Mar 9, 2020

krlmlr commented Mar 8, 2020 •

edited