-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fallback for validate_subclass()
#6119
Conversation
R/layer.R
Outdated
name <- snakeize(name) | ||
obj <- find_global(name, env = env, mode = "function") | ||
if (is.function(obj)) { | ||
obj <- obj() |
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.
Maybe wrap in try_fetch()
and rethrow the error with some additional context. We might end in situations where the constructor can't be called without arguments even though it generally should work
Merge branch 'main' into check_subclass_fallback # Conflicts: # R/layer.R
`environment()` must be either a string or a <geom> object, not an environment. | ||
|
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.
This seems like a regression in the error 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 argue the message has become more informative. The message comes from this test:
expect_snapshot_error(validate_subclass(environment(), "geom"))
And we now use caller_arg(x)
for messaging, so it'd self-describes the input.
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 - Ignore my pre-coffee eyes
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.
LGTM apart from the small comment
check_subclass()
validate_subclass()
This PR aims to progress #5962 and #3798.
Taking the deprecation of GeomCol (#3798) as an example, we want users to be able to use
layer(geom = "col", ...)
even after the class is deprecated. This PR ensures thatlayer(geom = "col")
finds thegeom_col()
constructor and returns thegeom
field from the resulting layer (even if we remove the GeomCol class).