Skip to content
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

bring error msg in line with allowed matrix/df cols as per #416 #465

Merged
merged 2 commits into from Aug 24, 2018

Conversation

@maxheld83
Copy link
Contributor

@maxheld83 maxheld83 commented Aug 23, 2018

Since matrix/df columns are now allowed, maybe this error msg should be changed accordingly, as well as respective tests.

Didn't mention df's explicitly, because those are, after all, lists.

@jennybc
Copy link
Member

@jennybc jennybc commented Aug 23, 2018

I've been staring at this message too, in my study of the name repair stuff.

If you look at the tests that exercise this, the "bogus" columns are NULL, an environment, and a quoted expression. Maybe it's safest to say 'something of this class is not allowed' and no longer attempt to give such a short description of what is allowed in the error message itself?

expect_error(
tibble(a = NULL),
error_column_must_be_vector("a", "NULL"),
fixed = TRUE
)
expect_error(
tibble(a = new.env()),
error_column_must_be_vector("a", "environment"),
fixed = TRUE
)
expect_error(
tibble(a = quote(a)),
error_column_must_be_vector("a", "name"),
fixed = TRUE
)

@maxheld83
Copy link
Contributor Author

@maxheld83 maxheld83 commented Aug 23, 2018

Great idea, that makes this short and sweet. (Well or sour, actually, since it’s an error message).

Do you think I should take another try at this?

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Aug 23, 2018

Thanks. Would "1d or 2d objects" be a sufficiently short and clear description? The error message needs to mention data frames anyway, they are treated differently from list columns.

We should agree on the wording before doing further implementation work.

@maxheld83
Copy link
Contributor Author

@maxheld83 maxheld83 commented Aug 23, 2018

I like the 1d/2d distinction a lot because (if you get it) it explains why which objects work and which don’t (they need to be vectors of equal length or nrows).
But I’m wondering how widespread this terminology is in R, especially to novices.
Guess not everyone will immediately know that a matrix is a 2d object/vector.

@jennybc
Copy link
Member

@jennybc jennybc commented Aug 23, 2018

Would "1d or 2d objects" be a sufficiently short and clear description?

Sounds good to me.

@codecov
Copy link

@codecov codecov bot commented Aug 24, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #465   +/-   ##
=======================================
  Coverage   90.83%   90.83%           
=======================================
  Files          26       26           
  Lines        1080     1080           
=======================================
  Hits          981      981           
  Misses         99       99
Impacted Files Coverage Δ
R/msg.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 997f5e2...fc613ce. Read the comment docs.

@maxheld83
Copy link
Contributor Author

@maxheld83 maxheld83 commented Aug 24, 2018

done!

@krlmlr krlmlr merged commit 11e3ab9 into tidyverse:master Aug 24, 2018
1 of 2 checks passed
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
Copy link
Member

@krlmlr krlmlr commented Aug 24, 2018

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants