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

Port to tidy evaluation (WIP) #225

Merged
merged 18 commits into from Apr 17, 2017

Conversation

Projects
None yet
3 participants
@lionel-
Member

lionel- commented Feb 10, 2017

Includes #224

This shouldn't be merged in until other packages are ported to tidyeval. lst_() is now deprecated in favour of the splicing idiom: lst(!!! alist(x, 2 * x)), or the tidyer lst(!!! list(~x, ~2 * x)).

@codecov-io

This comment has been minimized.

codecov-io commented Feb 17, 2017

Codecov Report

Merging #225 into master will decrease coverage by -6.91%.
The diff coverage is 59.42%.

@@            Coverage Diff            @@
##           master    #225      +/-   ##
=========================================
- Coverage   99.31%   92.4%   -6.91%     
=========================================
  Files          16      17       +1     
  Lines         725     790      +65     
=========================================
+ Hits          720     730      +10     
- Misses          5      60      +55
Impacted Files Coverage Δ
R/tibble.R 100% <100%> (ø)
R/enframe.R 100% <100%> (ø)
R/type-sum.r 100% <100%> (ø)
R/rownames.R 100% <100%> (ø)
R/utils.r 100% <100%> (ø)
R/tribble.R 100% <100%> (ø)
R/dataframe.R 100% <100%> (ø)
R/add.R 100% <100%> (ø)
R/utils-format.r 99.4% <100%> (ø)
R/tbl-df.r 100% <100%> (ø)
... and 5 more

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 4c32758...9dc79f2. Read the comment docs.

@lionel- lionel- force-pushed the lionel-:port-tidyeval branch from a35e855 to 72c5bbf Feb 17, 2017

@krlmlr

This comment has been minimized.

Member

krlmlr commented Feb 20, 2017

Is this ready for review?

@lionel-

This comment has been minimized.

Member

lionel- commented Feb 20, 2017

yes but this shouldn't be merged until tidyr and dplyr have been ported as well.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Feb 20, 2017

Why? (Just curious.)

@krlmlr

Thanks. I'll also play with your branch locally.

.travis.yml Outdated
@@ -11,7 +11,7 @@ r_github_packages:
- jimhester/covr
after_success:
- Rscript -e 'covr::codecov()'
- Rscript -e 'covr::codecov(line_exclusions = "compat-purrr.R")'

This comment has been minimized.

@krlmlr

krlmlr Feb 20, 2017

Member

Would you mind using exclusion markers in compat-purrr.R and keep this as is? Or is there another way to specify exclusions, via config file or the like?

names(output) <- character(n)
for (i in seq_len(n)) {
res <- lazyeval::lazy_eval(xs[[i]], output)
if (!is.null(res)) {
res <- rlang::tidy_eval(xs[[i]], discard_unnamed(output))

This comment has been minimized.

@krlmlr

krlmlr Feb 20, 2017

Member

Can we get rid of rlang::? Also, how/in which cases does the use of discard_unnamed() change the behavior of this function?

This comment has been minimized.

@krlmlr

krlmlr Feb 20, 2017

Member

I understand now. This looks fairly inefficient to me, can you change tidy_eval() so that it optionally ignores unnamed elements?

This comment has been minimized.

@lionel-

lionel- Feb 20, 2017

Member

yes that's already done locally actually.

This comment has been minimized.

@krlmlr

krlmlr Feb 20, 2017

Member

Can you get rid of discard_unnamed() then?

@@ -54,7 +53,7 @@ test_that("empty input makes 0 x 0 tbl_df", {
})
test_that("SE version", {
expect_identical(tibble_(list(a = ~1:10)), tibble(a = 1:10))
expect_warning(expect_identical(tibble_(list(a = ~1:10)), tibble(a = 1:10)))

This comment has been minimized.

@krlmlr

krlmlr Feb 20, 2017

Member

Can you please specify a warning expression?

@lionel-

This comment has been minimized.

Member

lionel- commented Feb 20, 2017

Why? (Just curious.)

Actually this just needs a compat function for lazy dots, then it should be fine.

@lionel- lionel- force-pushed the lionel-:port-tidyeval branch from 0d1ffa5 to 102a5a3 Mar 2, 2017

@lionel-

This comment has been minimized.

Member

lionel- commented Mar 2, 2017

Could you please take a look again @krlmlr

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 2, 2017

Thanks. I'd like to release tibble 1.3.0 first, this requires a readr update.

@lionel-

This comment has been minimized.

Member

lionel- commented Mar 2, 2017

The next version of tidyr depends on a tidyeval-enabled tibble, but since we are going to have a prerelease period for dplyr (on which tidyr depends) I guess this would still fit the CRAN release schedule? Do you know when the next dplyr is due?

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 2, 2017

I don't know; but when did you plan to release rlang?

To accelerate, I could as well bump tibble master to 1.3.10 and include your PR now, and release 1.3.0 when readr is on CRAN.

@lionel-

This comment has been minimized.

Member

lionel- commented Mar 2, 2017

I haven't discussed any release schedule with @hadley

If I had to guess, it would make sense to aim for everything released by mid-April, but I don't know.

Is the customary CRAN release gap 1 month or 2 months?

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 2, 2017

I've been pushing RSQLite releases on a weekly basis with good reason; the gap is more a suggestion. I guess they just don't want you to push every trivial update.

@lionel-

This comment has been minimized.

Member

lionel- commented Mar 2, 2017

Ok, no worries then, we can keep the tidyeval update in this branch.

@krlmlr krlmlr merged commit 9dc79f2 into tidyverse:master Apr 17, 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 added a commit that referenced this pull request Apr 17, 2017

Merge branch 'f-port-tidyeval'. Closes #225.
- Using `rlang` instead of `lazyeval` (#225, @lionel-).

krlmlr added a commit that referenced this pull request Apr 19, 2017

Merge tag 'v1.3.0.9001'
- 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 (#235).
- Minor efficiency gains by replacing `any(is.na())` with `anyNA()` (#229, @csgillespie).
- Using `rlang` instead of `lazyeval` (#225, @lionel-).

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