-
Notifications
You must be signed in to change notification settings - Fork 418
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
Update replace_na()
to utilize vctrs
#1219
Update replace_na()
to utilize vctrs
#1219
Conversation
Caught a few broken revdeps from this. I still think it is worth pushing forward with this change, sending PRs as required, because these look like real coding errors that we could help fix, and 7 PRs / issues isn't that many. Thoughts @hadley? (It is especially worth moving forward with this if we also upgrade Summary:
|
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.
If you want to do the work on the revdeps, I think it's reasonable to fix this.
Prior to release, we should also spend a little time running down the "failures to check" and see if we can align more closely to CRAN.
col_arg <- col_args[[i]] | ||
value_arg <- value_args[[i]] | ||
|
||
check_replacement(value, col_arg) |
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.
Do we still need this? Or can we rely on vec_assign()
to apply the recycling rules appropriately?
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 think we still need this, just because of the way replace_na()
works.
Otherwise the replace
value would have to be either size 1 or the same size as sum(is.na(data))
, and that is almost never the case because you don't know how many missing values there are ahead of time.
x <- c(1, 2, NA, 3, NA)
# This works, of course
replace_na(x, 0)
#> [1] 1 2 0 3 0
# This would actually work too if we removed the restriction
# (because there are two missing values)
# (I don't think we want this to work)
replace_na(x, c(0, -1))
#> [1] 1 2 0 3 -1
# This would not work
# (because replace_na() wants to replace with something of length 2)
y <- c(1, 2, 3, 4, 5)
replace_na(x, y)
# Can't recycle `replace` (size 5) to size 2.
d181a0d
to
04493b5
Compare
Revival of #1109
Closes #1168
Closes #912 (by mentioning that you should use
across()
in the NEWS bullet)This PR updates
replace_na()
to utilize vctrs throughvec_equal_na()
andvec_assign()
. This has two breaking changes:vec_assign()
always casts the RHS to the LHS, but previously with[<-
the reverse could be true. I think this is a step in the right directionreplace = 1.5
on an integer vectorreplace = "foo"
, it must bereplace = list("foo")
vec_equal_na()
is now used for detecting missing values. For list-cols, it will only detectNULL
as missing, while the current implementation will replaceNULL
and empty atomics likeinteger()
. Again, I think this is a net positive.