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

Informative error message for as_tibble() when rownames is missing #388

Merged
merged 1 commit into from May 23, 2018

Conversation

Projects
None yet
2 participants
@anhqle
Copy link
Contributor

anhqle commented Feb 28, 2018

as_tibble() now throws an error when user wants to convert row names into a column but row names is missing. Close #386.

Question: I wanted the error message to mention the object name, i.e. "Object my_matrix does not have existing row names"). However, I can't get rlang::enexpr() to capture the user-submitted expression. Instead, rlang::enexpr() returns the object.

I think that this happens because when the expression goes through the generic function, the promise is already evaluated and can no longer be captured once it reaches as_tibble.data.frame. Is understanding correct?

If so, how can we capture the user-submitted expression?

as_tibble() now throws an error when user wants to convert row names …
…into a column but row names is missing. Close #386.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 28, 2018

Codecov Report

Merging #388 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   90.39%   90.42%   +0.02%     
==========================================
  Files          23       23              
  Lines        1083     1086       +3     
==========================================
+ Hits          979      982       +3     
  Misses        104      104
Impacted Files Coverage Δ
R/as_tibble.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 80ac1a3...0efcdda. Read the comment docs.

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Mar 1, 2018

Perhaps we could try declaring the generic like this:

as_tibble <- function(x, ..., .expr = enexpr(x)) ...

Throwing an error here is an API change. What is wrong with the current behavior? In the issue you referred to, as_tibble.matrix() seems to be the problem.

@anhqle

This comment has been minimized.

Copy link
Contributor

anhqle commented Mar 1, 2018

as_tibble.matrix() coerces the matrix into a data frame, then ultimately use as_tibble.data.frame(). (See source). Hence the fix is done inside as_tibble.data.frame().

Furthermore, the same reported bug also happens to data frame, not just matrices.

> mydf <- data.frame(a = 1:3)
> as_tibble(mydf, rownames = "myrowname")
Error: `.data` must have 3 rows, not 2 

The current behavior is wrong in 2 ways:

  1. It throws an uninformative error as demonstrated above. It should have said the error is due to missing row name

  2. It gives wrong result when mydf coincidentally has 2 rows. This allows the result of .row_names_info, which is a length-2 vector, to be turned into a column.

> mydf <- data.frame(a = 1:2)
> as_tibble(mydf, rownames = "myrowname")
# A tibble: 2 x 2
  myrowname     a
      <int> <int>
1        NA     1
2        -2     2
@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Mar 6, 2018

I see what you mean. This must have been a regression that sneaked in and nobody noticed.

@anhqle

This comment has been minimized.

Copy link
Contributor

anhqle commented Apr 23, 2018

@krlmlr Is there anything change you want to make here before merging?

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Apr 23, 2018

Thanks. The PR seems fine, I'll merge when I work on tibble again.

@krlmlr krlmlr merged commit 8ad7706 into tidyverse:master May 23, 2018

4 checks passed

codecov/patch 100% of diff hit (target 90.39%)
Details
codecov/project 90.42% (+0.02%) compared to 80ac1a3
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.

Copy link
Member

krlmlr commented May 23, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment