tribble now handles columns with a class#237
Conversation
krlmlr
left a comment
krlmlr
left a comment
There was a problem hiding this comment.
Thanks! Looks very good already, just a few minor remarks.
| } else { | ||
| frame_col[[i]] <- unlist(col) | ||
| } else if (inherits(col, "list")){ | ||
| frame_col[[i]] <- do.call(c, col) |
There was a problem hiding this comment.
Could you please use invoke() from rlang instead?
There was a problem hiding this comment.
Done and pushed. Wasn't aware of rlang::invoke - looks good.
| *.o | ||
| src/*.o-* | ||
| src-i386/ | ||
| src-x64/ |
There was a problem hiding this comment.
On Windows, x64 building the package generates these folders with binary output. They're not needed in the version control, so I've added them to .gitignore. If you'd prefer to keep them out of .gitignore, I could of course just ignore them manually!
There was a problem hiding this comment.
I don't mind having them in .gitignore.
| sys_time <- Sys.time() | ||
| date_time_col <- tribble( | ||
| ~dt, ~dttm, | ||
| sys_date, sys_time |
There was a problem hiding this comment.
Could you please create more than one row in the test?
| @@ -1,3 +1,7 @@ | |||
| ## tibble 1.3.0.9001 (2017-04-18) | |||
|
|
|||
| - `tribble` now handles columns with class andles other columns with a class. (#161, @NikNakk) | |||
There was a problem hiding this comment.
Happy to use a NEWS entry you provide in a comment, but I prefer to change NEWS myself.
There was a problem hiding this comment.
Fair enough. Have reverted to previous NEWS.md. I was following what Hadley had asked me to do for previous PR to dplyr.
Codecov Report
@@ Coverage Diff @@
## master #237 +/- ##
=======================================
Coverage 89.48% 89.48%
=======================================
Files 21 21
Lines 875 875
=======================================
Hits 783 783
Misses 92 92
Continue to review full report at Codecov.
|
|
GitHub seems to have trouble separating your changes from those in master. Could you please rebase against current master and then force-push? |
|
I did that last night - github says above there are no conflicts with the base branch. Is there still an issue? The only fail I can seem above is an AppVeyor fail related to lack of |
|
If you look at the diff, you'll see many changes that are unrelated to your PR. I think you need: git checkout fix-other-class-columns
git fetch --all
git rebase origin/master
git push # will show an error
git push --force |
As per request from krlmlr in tribble.R Also added two line test to test-tribble.R
Fixes tidyverse#161, and allows for syntax such as: tribble(~a, Sys.Date() ) Also updated NEWS, DESCRIPTION and test-tribble accordingly.
NikNakk
force-pushed
the
fix-other-class-columns
branch
from
0ce9956 to
fe285ae
Compare
Simplified checks for whether to change list to vector so that only two branches to `if` needed
|
Sorry, I thought that's what I'd done yesterday, and the diff seemed to have come down in size, but anyway it's redone now. I've also realised I could simplify the |
krlmlr
left a comment
krlmlr
left a comment
There was a problem hiding this comment.
Thanks, almost ready to merge.
| frame_col[[i]] <- col | ||
| } else { | ||
| frame_col[[i]] <- unlist(col) | ||
| frame_col[[i]] <- rlang::invoke(c, col) |
There was a problem hiding this comment.
I don't think we need rlang::, because we import the entire package.
|
Thanks. |
|
Thanks! |
- New `rowid_to_column()` that adds a `rowid` column as first column and removes row names (#243, @barnettjacob). - Use `rlang` functions (#244). - The `all.equal.tbl_df()` method has been removed, calling `all.equal()` now forwards to `base::all.equal.data.frame()`. To compare tibbles ignoring row and column order, please use `dplyr::all_equal()` (#247). - The `microbenchmark` package is now used conditionaly (#245). - Subsetting zero columns no longer returns wrong number of rows (#241, @echasnovski). - `tribble()` now handles values that have a class (#237, @NikNakk).
- Subsetting zero columns no longer returns wrong number of rows (#241, @echasnovski). - New `set_tidy_names()` and `tidy_names()`, a simpler version of `repair_names()` which works unchanged for now (#217). - New `rowid_to_column()` that adds a `rowid` column as first column and removes row names (#243, @barnettjacob). - The `all.equal.tbl_df()` method has been removed, calling `all.equal()` now forwards to `base::all.equal.data.frame()`. To compare tibbles ignoring row and column order, please use `dplyr::all_equal()` (#247). - Printing now uses `x` again instead of the Unicode multiplication sign, to avoid encoding issues (#216). - String values are now quoted when printing if they contain non-printable characters or quotes (#253). - The `print()`, `format()`, and `tbl_sum()` methods are now implemented for class `"tbl"` and not for `"tbl_df"`. This allows subclasses to use tibble's formatting facilities. The formatting of the header can be tweaked by implementing `tbl_sum()` for the subclass, which is expected to return a named character vector. The `print.tbl_df()` method is still implemented for compatibility with downstream packages, but only calls `NextMethod()`. - Own printing routine, not relying on `print.data.frame()` anymore. Now providing `format.tbl_df()` and full support for Unicode characters in names and data, also for `glimpse()` (#235). - Improve formatting of error messages (#223). - Using `rlang` instead of `lazyeval` (#225, @lionel-), and `rlang` functions (#244). - `tribble()` now handles values that have a class (#237, @NikNakk). - Minor efficiency gains by replacing `any(is.na())` with `anyNA()` (#229, @csgillespie). - The `microbenchmark` package is now used conditionally (#245). - `pkgdown` website.
3 participants
Fixes #161, and allows for syntax such as:
Also updated NEWS, DESCRIPTION and
test-tribble accordingly.