Define responsibilities of new_tibble() #332
Conversation
Codecov Report
@@ Coverage Diff @@
## master #332 +/- ##
==========================================
+ Coverage 88.9% 89.08% +0.17%
==========================================
Files 24 24
Lines 1109 1127 +18
==========================================
+ Hits 986 1004 +18
Misses 123 123
Continue to review full report at Codecov.
|
|
I think it should at least require that |
|
I was confused by your original comment in #211, agree that |
krlmlr
force-pushed
the
f-new-tibble-responsibilities
branch
from
e8be412 to
3b6aa24
Compare
|
But then adv-r says there should be an ellipsis, thanks @DavisVaughan:
Added it back, I think the code is a bit cleaner now. |
krlmlr
force-pushed
the
f-new-tibble-responsibilities
branch
from
bb44ed6 to
eb64ffa
Compare
R/new.R
Outdated
| #' @param subclass Subclasses to assign to the new object, default: none | ||
| #' @export | ||
| new_tibble <- function(x, ..., subclass = NULL) { | ||
| new_tibble <- function(x, ..., nrow = NULL, subclass = NULL) { |
There was a problem hiding this comment.
I think this needs a bit more documentation, i.e.:
xmust be a named list (but names are not checked for correctness).- The
rownamesattribute is created automatically - The
classis set to ...
R/new.R
Outdated
| x <- update_tibble_attrs(x, ...) | ||
| x <- set_tibble_class(x, subclass = subclass) | ||
| # Make sure that we override any row names that | ||
| # may have been there previously, in x or in ... | ||
| if (is.null(nrow)) nrow <- guess_nrow(x) |
There was a problem hiding this comment.
The comment doesn't seem right to me - I think it should just be "set row names" (in which case you can just remove it)
|
Do you want to take another look? |
R/new.R
Outdated
| new_tibble <- function(x, ..., subclass = NULL) { | ||
| #' @examples | ||
| #' new_tibble(list(a = 1:3, b = 4:6)) | ||
| #' new_tibble(list(), nrow = 150, subclass = "my_tibble") |
There was a problem hiding this comment.
This is a little subtle without a comment
R/new.R
Outdated
| if (is.null(nrow)) nrow <- guess_nrow(x) | ||
| attr(x, "row.names") <- .set_row_names(nrow) | ||
|
|
||
| #' The `new_tibble()` constructor makes sure that the `row.names` attribute |
@hadley: What should
new_tibble()check or enforce? We haveas_tibble.list()that will check column names and recycle columns if necessary, but no attributes are copied. Shouldnew_tibble()compute therow.namesattribute if necessary and do additional checks?Reference: #330 (comment)