-
Notifications
You must be signed in to change notification settings - Fork 71
Improve on scalar object error #2062
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
Conversation
| #' @name howto-faq-fix-scalar-type-error | ||
| #' @aliases howto_faq_fix_scalar_type_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created underscored aliases for these for use in the cli bullet. In cli's fallback case it shows ?faq_error_scalar_type which doesn't work well with - if you try and copy and paste it in an R console. We've also learned this in the past with ?rlang::args_dots_empty
R/conditions.R
Outdated
| message <- glue::glue("{arg} must be a vector, not {obj_type_friendly(x)}.") | ||
|
|
||
| if (needs_incompatible_s3_list_bullets(x)) { | ||
| message <- with_incompatible_s3_list_bullets(x, message) | ||
| } else if (needs_incompatible_data_frame_bullets(x)) { | ||
| message <- with_incompatible_data_frame_bullets(x, message) | ||
| } else { | ||
| message <- with_scalar_faq_bullet(message) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be tempted to write it like this, where the individual functions know if they're needed or not.
| message <- glue::glue("{arg} must be a vector, not {obj_type_friendly(x)}.") | |
| if (needs_incompatible_s3_list_bullets(x)) { | |
| message <- with_incompatible_s3_list_bullets(x, message) | |
| } else if (needs_incompatible_data_frame_bullets(x)) { | |
| message <- with_incompatible_data_frame_bullets(x, message) | |
| } else { | |
| message <- with_scalar_faq_bullet(message) | |
| } | |
| message <- c( | |
| glue::glue("{arg} must be a vector, not {obj_type_friendly(x)}."), | |
| incompatible_s3_list_bullets(x), | |
| incompatible_data_frame_bullets(x), | |
| scalar_faq_bullet() | |
| ) |
(probably tweaking the function names to make it clear they're only generated it needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it's the last one that's the problem. I only want a scalar faq bullet if we don't have a more specific bullet to show first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incompatible_s3_list_bullets(x) %||%
incompatible_data_frame_bullets(x) %||%
scalar_faq_bullet()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's pretty good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
R/conditions.R
Outdated
| "Detected incompatible data frame subclass.", | ||
| "To be a vector, the subclass must come before {.cls data.frame} in the", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That message is a bit confusing to me. Maybe it's worth expanding a bit:
| "Detected incompatible data frame subclass.", | |
| "To be a vector, the subclass must come before {.cls data.frame} in the", | |
| "Detected unexpected data frame structure.", | |
| "A data frame is normally treated as vector but the detection was thrown off by an unexpected subclass ordering. To be compatible with vctrs, a data frame subclass must come before {.cls data.frame} in the", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think "incompatible" makes it seem like a valid structure, so "unexpected" is probably better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've revised the data frame message in d4a7383 but I do think "incompatible" is still right. It's incompatible with vctrs. I don't want to mention vctrs in the error though, because this error could get thrown from any tidyverse function and the error_call won't mention a vctrs function.
What do you think of the new message?
fbd201f to
f53db02
Compare
Closes #2061
@MilesMcBain it turns out we already have these two useful FAQ pages:
Mimicking some ideas from the Rust compiler, we can link directly to these with cli from the error message
We now throw 3 distinct error messages - @lionel- and @hadley this is your chance to critique these
Interactively, cli shows the links as clickable links
It's a bit annoying to improve on
x must be a vector, not a <data.frame> object.without more work than I'm willing to put in, but I think the rest of the message clears up this edge case.