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

add_column where .data is a tibble with 0 columns produces an unexpected, undocumented result #319

Closed
dan87134 opened this Issue Oct 14, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@dan87134

dan87134 commented Oct 14, 2017

I've posted this to the https://community.rstudio.com/c/tidyverse discussion in answer to a question I had asked about why add_column could not add a column to a tibble that had 0 columns (tibble())

I've come to the conclusion that add_column is producing an undocumented and unexpected result when .data (the input tibble) has 0 columns.

Here is why I think this...

I checked the source for add_column and it checks for a number of edge cases, for example where the number of added columns is 0, but the case where .data (i.e. the input tibble) has 0 columns is not checked.

This produces a the confusing error "Error: 0 is not TRUE", which I would call a programming surprise because does not look to be intentional :-) .

This happens because later in the code pluralise_n is used with nrow(.data), i.e. pluralise_n is used with 0,.to make an error message. pluralise_n produces the low level message ""Error: 0 is not TRUE" when it is asked to pluralize 0.

However, after a bit of checking and setup, all that add_column does is concatenate the added columns with the ones already in the tibble. This make sense because a tibble stores it's content as a sequence of columns.

So... it would make sense (IMHO of course) for add_column to check to see if the .data (the input tibble) contained 0 columns and if it did just return the input columns (i.e ...) as a new tibble. I think that this kind of behavior would be more consistent with name and implied behavior of a function named add_column.

Supporting to a 0 column .data could be added with a simple test at the beginning of the code.

#existing code
if (ncol(df) == 0L) {
return(.data)
}
#add this to support 0 columns .data
if(ncol(.data)==0L) {
return(df)
}

@krlmlr krlmlr closed this in 39b1823 Oct 15, 2017

@krlmlr

This comment has been minimized.

Member

krlmlr commented Oct 15, 2017

Thanks. I've only skimmed over the original thread, and I think that add_column() should never change the number of rows. The error message was bad, I fixed that.

I would consider a PR that adds a function that constructs a zero-column tibble with a fixed number of rows.

Note that the new tidyeval framework permits much more elegant solutions:

library(tibble)
x <- list(a = 1:3, b = 4:6)
tibble(!!! x)
#> # A tibble: 3 x 2
#>       a     b
#>   <int> <int>
#> 1     1     4
#> 2     2     5
#> 3     3     6
@dan87134

This comment has been minimized.

dan87134 commented Oct 16, 2017

Thanks for the pointer to triple wack. It cleans up things for me... it's less typing than do.call :-) I really have to dig into tidyeval now.

krlmlr added a commit that referenced this issue Nov 3, 2017

Merge tag 'v1.3.4.9002'
- In `glimpse()`, compute `type_sum()` from data frame for dbplyr compatibility (#328).
- `as_tibble.matrix()` repairs column names.
- Compatible with R 3.1 (#323).
- Make add_case() and alias for add_row() (#324, @LaDilettante).
- `add_column()` to an empty zero-row tibble with a variable of nonzero length now produces a correct error message (#319).
- Logical indexes are supported, a warning is raised if the length does not match the number of rows or 1 (#318).
- Tibbles now support character subsetting (#312).
- `as_tibble()` gains `rownames` argument (#288, #289).
- Remove Rcpp dependency (#313, @patperry).
- ``` `[.tbl_df`() ``` supports `drop = TRUE` and omits the warning if `j` is passed. The calls `df[i, j, drop = TRUE]` and `df[i, drop = TRUE]` are now compatible with data frames again (#307, #311).
- Prevent `add_column()` from dropping classes and attributes by removing the use of `cbind()`. Additionally this ensures that `add_column()` can be used with grouped data frames (#303, @DavisVaughan).
- Fixed width for word wrapping of the extra information (#301).

krlmlr added a commit that referenced this issue Dec 28, 2017

Merge tag 'v1.4.1'
New formatting
--------------

The new pillar package is now responsible for formatting tibbles. Pillar will try to display as many columns as possible, if necessary truncating or shortening the output. Colored output highlights important information and guides the eye. The vignette in the tibble package describes how to adapt custom data types for optimal display in a tibble.

New features
------------

- Make `add_case()` an alias for `add_row()` (#324, @LaDilettante).
- `as_tibble()` gains `rownames` argument (#288, #289).
- `as_tibble.matrix()` repairs column names.
- Tibbles now support character subsetting (#312).
- ``` `[.tbl_df`() ``` supports `drop = TRUE` and omits the warning if `j` is passed. The calls `df[i, j, drop = TRUE]` and `df[i, drop = TRUE]` are now compatible with data frames again (#307, #311).

Bug fixes
---------

- Improved compatibility with remote data sources for `glimpse()` (#328).
- Logical indexes are supported, a warning is raised if the length does not match the number of rows or 1 (#318).
- Fixed width for word wrapping of the extra information (#301).
- Prevent `add_column()` from dropping classes and attributes by removing the use of `cbind()`. Additionally this ensures that `add_column()` can be used with grouped data frames (#303, @DavisVaughan).
- `add_column()` to an empty zero-row tibble with a variable of nonzero length now produces a correct error message (#319).

Internal changes
----------------

- Reexporting `has_name()` from rlang, instead of forwarding, to avoid warning when importing both rlang and tibble.
- Compatible with R 3.1 (#323).
- Remove Rcpp dependency (#313, @patperry).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment