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 should ignore trailing commas #342

Merged
merged 2 commits into from Jan 15, 2018

Conversation

Projects
None yet
3 participants
@anhqle
Contributor

anhqle commented Dec 18, 2017

Close #338

Idea:

  • Use sys.call() to get the list of function arguments
  • Get rid of trailing commas
  • Pass the remaining arguments forward

Problem (need help):

If there are unevaluated expressions in the list of arguments, I don't know how to keep track of the environment where they are created and should be evaluated in. For example, the following test case currently fails:

test_that("tribble() handles columns with a class (#161)", {
  sys_date <- Sys.Date()
  sys_time <- Sys.time()
  date_time_col <- tribble(
    ~dt, ~dttm,
    sys_date, sys_time,
    as.Date("2003-01-02"), as.POSIXct("2004-04-05 13:45:17", tz = "UTC")
  )

  <<rest is omitted>>
})

extract_frame_data_from_dots is not intelligent enough to look for sys_date and sys_time by traversing up the tree of calling environments. How to do this?

@lionel-

This comment has been minimized.

Member

lionel- commented Dec 18, 2017

Taking the dots with dots_list() instead of list() should be enough to ignore trailing commas.

@lionel-

This comment has been minimized.

@codecov

This comment has been minimized.

codecov bot commented Dec 19, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #342   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files          24       24           
  Lines        1122     1122           
=======================================
  Hits          981      981           
  Misses        141      141
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 60281b3...7462722. Read the comment docs.

@anhqle

This comment has been minimized.

Contributor

anhqle commented Dec 19, 2017

@lionel- Thanks a lot for the tip! It works perfectly.

@krlmlr krlmlr merged commit dd08491 into tidyverse:master Jan 15, 2018

4 checks passed

codecov/patch 100% of diff hit (target 87.43%)
Details
codecov/project 87.43% (+0%) compared to 60281b3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@krlmlr

This comment has been minimized.

Member

krlmlr commented Jan 15, 2018

Thanks!

@anhqle anhqle deleted the anhqle:338-tribble-ignores-trailing-commas branch Jan 15, 2018

krlmlr added a commit that referenced this pull request Jan 20, 2018

Merge tag 'v1.4.1.9001'
- `enframe(NULL)` now returns the same as `enframe(logical())` (#352).
- `tbl[1, , drop = TRUE]` now behaves identically to data frames (#367).
- The `tibble.width` option is honored again (#369).
- Faster printing of very wide tibbles (#360).
- Update vignette to match changes in 1.4.1 (#368, @bgreenwell).
- Don't rely on `ncol()` for `glimpse()`, only query `nrow()` and `head()`.
- Return input for zero-column data frames.
- Add test for `glimpse()` with unknown rows (#366, @kevinykuo).
- Faster construction and subsetting for tibbles (#353).
- `tribble()` now ignores trailing commas (#342, @LaDilettante).
- Fix error message when accessing columns using a logical index vector (#337, @mundl).

krlmlr added a commit that referenced this pull request Jan 23, 2018

Merge tag 'v1.4.2'
Bug fixes
---------

- Fix OS X builds.
- The `tibble.width` option is honored again (#369).
- `tbl[1, , drop = TRUE]` now behaves identically to data frames (#367).
- Fix error message when accessing columns using a logical index vector (#337, @mundl).
- `glimpse()` returns its input for zero-column data frames.

Features
--------

- `enframe(NULL)` now returns the same as `enframe(logical())` (#352).
- `tribble()` now ignores trailing commas (#342, @LaDilettante).
- Updated vignettes and website documentation.

Performance
-----------

- Faster printing of very wide tibbles (#360).
- Faster construction and subsetting for tibbles (#353).
- Only call `nrow()` and `head()` in `glimpse()`, not `ncol()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment