-
Notifications
You must be signed in to change notification settings - Fork 273
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
modify
fails when .f
returns NULL
#753
Comments
@@ -136,7 +136,11 @@ modify.default <- function(.x, .f, ...) {
.f <- as_mapper(.f, ...)
for (i in seq_along(.x)) {
- .x[[i]] <- .f(.x[[i]], ...)
+ el <- .f(.x[[i]], ...)
+ if (is.null(el))
+ .x[i] <- list(NULL)
+ else
+ .x[[i]] <- el
}
.x |
Perhaps use the same strategy as in r-lib/rlang#947, and check if input is a list before using special treatment of |
Isn't modify.default called only on recursives anyhow? So special treatment of lists doesn't make much sense here. BTW, modify doesn't work on environments, might be good to add on this occasion. |
Ok, this is a bit more complex than I though. The only way to assign NULL to a list-like is to use Do you have an internal low level assignment in the package? Also preserving NULL on data.frame is meaningless. Do you want to keep the same-length invariant for data.frames, or it would be ok to remove the column? I guess the question is, do you want NULL from .f to eliminate the value or to insert NULL. If the latter then the modifier should break for classes for which it doesn't make sense (like data.frame). |
It will also be called on classed atomic vectors.
Environments are collections but not vectors, so I don't think we should make it work.
Dispatching on
This will be vctrs. It might be better to wait for the switch to vctrs actually, since there are tricky aspects of type here. |
Dare to elaborate? Why would type play a role for a generic modifier? If it makes sense, then just do it IMO.
I have a simple solution for now. Will PR in a couple of minutes. |
purrr::modify.default
should check explicitly for null valuesCreated on 2020-03-18 by the reprex package (v0.3.0)
Likely duplicates #655 and #746. Creating the new issue with simple reprex to create a sense of urgency :) If a simple check for NULL in modify.default is ok with you I can create a PR and add tests.
The text was updated successfully, but these errors were encountered: