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

`tribble` now handles columns with a class #237

Merged
merged 4 commits into from Apr 21, 2017

Conversation

Projects
None yet
3 participants
@NikNakk

NikNakk commented Apr 18, 2017

Fixes #161, and allows for syntax such as:

tribble(~a,
        Sys.Date()
       )

Also updated NEWS, DESCRIPTION and
test-tribble accordingly.

@krlmlr

Thanks! Looks very good already, just a few minor remarks.

R/tribble.R Outdated
} else {
frame_col[[i]] <- unlist(col)
} else if (inherits(col, "list")){
frame_col[[i]] <- do.call(c, col)

This comment has been minimized.

@krlmlr

krlmlr Apr 19, 2017

Member

Could you please use invoke() from rlang instead?

This comment has been minimized.

@NikNakk

NikNakk Apr 19, 2017

Done and pushed. Wasn't aware of rlang::invoke - looks good.

@@ -1,5 +1,7 @@
*.o
src/*.o-*
src-i386/
src-x64/

This comment has been minimized.

@krlmlr

krlmlr Apr 19, 2017

Member

Why is this necessary?

This comment has been minimized.

@NikNakk

NikNakk Apr 19, 2017

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!

This comment has been minimized.

@krlmlr

krlmlr Apr 20, 2017

Member

I don't mind having them in .gitignore.

sys_time <- Sys.time()
date_time_col <- tribble(
~dt, ~dttm,
sys_date, sys_time

This comment has been minimized.

@krlmlr

krlmlr Apr 19, 2017

Member

Could you please create more than one row in the test?

NEWS.md Outdated
@@ -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)

This comment has been minimized.

@krlmlr

krlmlr Apr 19, 2017

Member

Happy to use a NEWS entry you provide in a comment, but I prefer to change NEWS myself.

This comment has been minimized.

@NikNakk

NikNakk Apr 19, 2017

Fair enough. Have reverted to previous NEWS.md. I was following what Hadley had asked me to do for previous PR to dplyr.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 19, 2017

Codecov Report

Merging #237 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #237   +/-   ##
=======================================
  Coverage   89.48%   89.48%           
=======================================
  Files          21       21           
  Lines         875      875           
=======================================
  Hits          783      783           
  Misses         92       92
Impacted Files Coverage Δ
R/tribble.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc87659...19e3347. Read the comment docs.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Apr 20, 2017

GitHub seems to have trouble separating your changes from those in master. Could you please rebase against current master and then force-push?

@NikNakk

This comment has been minimized.

NikNakk commented Apr 20, 2017

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 rlang for R 3.1.3.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Apr 20, 2017

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

Nick Kennedy added some commits Apr 19, 2017

Nick Kennedy
Changed `do.call` to `rlang::invoke`
As per request from krlmlr in tribble.R

Also added two line test to test-tribble.R
Nick Kennedy
`tribble` now handles columns with a class
Fixes #161, and allows for syntax such as:

    tribble(~a,
            Sys.Date()
           )

Also updated NEWS, DESCRIPTION and
test-tribble accordingly.

@NikNakk NikNakk force-pushed the NikNakk:fix-other-class-columns branch from 0ce9956 to fe285ae Apr 20, 2017

Nick Kennedy
Simplified tribble.R
Simplified checks for whether to change list to
vector so that only two branches to `if` needed
@NikNakk

This comment has been minimized.

NikNakk commented Apr 20, 2017

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 if in turn_matrix_into_column_list.

@krlmlr

Thanks, almost ready to merge.

R/tribble.R Outdated
frame_col[[i]] <- col
} else {
frame_col[[i]] <- unlist(col)
frame_col[[i]] <- rlang::invoke(c, col)

This comment has been minimized.

@krlmlr

krlmlr Apr 20, 2017

Member

I don't think we need rlang::, because we import the entire package.

@NikNakk

This comment has been minimized.

NikNakk commented Apr 20, 2017

Thanks. rlang:: removed.

@krlmlr krlmlr merged commit 08af6b0 into tidyverse:master Apr 21, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@krlmlr

This comment has been minimized.

Member

krlmlr commented Apr 21, 2017

Thanks!

krlmlr added a commit that referenced this pull request May 8, 2017

Merge tag 'v1.3.0.9002'
- 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).

krlmlr added a commit that referenced this pull request May 17, 2017

Merge tag 'v1.3.1'
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment