Is new_tibble() working as it should?

[DavisVaughan]

Hey @krlmlr, I'm following the work on extending tibbles pretty closely and saw the new_tibble() function pop up in the commits.

Playing around with it, it does not seem to work as expected.

library(tibble)

iris_tbl <- as.tibble(iris)

# The attribute is given the name "name"
iris_with_attr <- new_tibble(iris_tbl, new_attr = 4)
attributes(iris_with_attr)
#> $names
#> [1] "Sepal.Length" "Sepal.Width"  "Petal.Length" "Petal.Width" 
#> [5] "Species"     
#> 
#> $row.names
#>   [1]   1   2   3   4   5   6   7   8   9  10  11  12  13  14  15  16  17
#>  [18]  18  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34
#>  [35]  35  36  37  38  39  40  41  42  43  44  45  46  47  48  49  50  51
#>  [52]  52  53  54  55  56  57  58  59  60  61  62  63  64  65  66  67  68
#>  [69]  69  70  71  72  73  74  75  76  77  78  79  80  81  82  83  84  85
#>  [86]  86  87  88  89  90  91  92  93  94  95  96  97  98  99 100 101 102
#> [103] 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119
#> [120] 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136
#> [137] 137 138 139 140 141 142 143 144 145 146 147 148 149 150
#> 
#> $class
#> [1] "tbl_df"     "tbl"        "data.frame"
#> 
#> $name
#> [1] 4

# Both attributes are given the name "name" so only the second one is kept
# and the first is overwritten
iris_with_2_attrs <- new_tibble(iris_tbl, new_attr1 = 4, new_attr2 = 8)
attributes(iris_with_2_attrs)
#> $names
#> [1] "Sepal.Length" "Sepal.Width"  "Petal.Length" "Petal.Width" 
#> [5] "Species"     
#> 
#> $row.names
#>   [1]   1   2   3   4   5   6   7   8   9  10  11  12  13  14  15  16  17
#>  [18]  18  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34
#>  [35]  35  36  37  38  39  40  41  42  43  44  45  46  47  48  49  50  51
#>  [52]  52  53  54  55  56  57  58  59  60  61  62  63  64  65  66  67  68
#>  [69]  69  70  71  72  73  74  75  76  77  78  79  80  81  82  83  84  85
#>  [86]  86  87  88  89  90  91  92  93  94  95  96  97  98  99 100 101 102
#> [103] 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119
#> [120] 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136
#> [137] 137 138 139 140 141 142 143 144 145 146 147 148 149 150
#> 
#> $class
#> [1] "tbl_df"     "tbl"        "data.frame"
#> 
#> $name
#> [1] 8

I think it's due to the map2 call. I can see that you are using a version that just calls Map. Somehow list(name = value) is actually using "name" as the name of the list element.

I would submit a PR with a fix but I'm not entirely sure what you are trying to do. My first thought was:

new_tibble <- function (x, ..., subclass = NULL) {
  attribs <- list(...)
  x <- rlang::set_attrs(x, rlang::splice(attribs))

  class(x) <- c(subclass, "tbl_df", "tbl", "data.frame")

  x
}

[DavisVaughan]

Additionally, to satisfy the standards Hadley seems to be trying to set, maybe it should include stopifnot(is.data.frame(x)) as the first line.

See #275

[krlmlr]

Thanks. What a blunder...

As for the checks, we'd need to define the responsibilities of new_tibble(). Should we also check for names and consistency of column length?

[DavisVaughan]

From my interpretation of Advanced R, the constructors should only be involved in 3 things:

1. Be called new_class_name().
2. Have one argument for the base object, and one for each attribute.
3. Check the types of the base object and each attribute.

The checks for names and column length consistency might be better suited for a validate_tibble() function since they don't deal with the type of x. I like the example with factors here.

Reasoning:
"Rather than encumbering the constructor with complicated checks, it’s better to put them in a separate function. This is a good idea because it allows you to cheaply create new objects when you know that the values are correct, and to re-use the checks in other places."

Would that make sense?

[github-actions]

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.