-
Notifications
You must be signed in to change notification settings - Fork 189
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
Vectorise replacement function #462
Conversation
It also breaks rmarkdown 😬
|
rmarkdown patch in rstudio/rmarkdown#2416. Breaking rmarkdown is a big deal, so I don't think we should merge this until after rmarkdown is released, so it probably won't make it for this release. |
44d861e
to
4767c88
Compare
I guess pkgdown is failing because rmarkdown is broken? Very meta - ah yes i see that in your backtrace |
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 guess the broken packages are like rmarkdown? They didn't expect multiple strings at once?
R/replace.R
Outdated
|
||
# unchop list into a vector, apply replacement(), and then rechop back into | ||
# a list | ||
old_flat <- vctrs::vec_unchop(old, idx) |
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.
Depending on how much you value readability over performance, you don't actually need to pass idx
here since you are unchopping in the order the input was provided
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.
In dev vctrs:
xs <- list("a", c("b", "c"), c("d", "e", "f"))
x <- list_unchop(xs)
x <- toupper(x)
x
#> [1] "A" "B" "C" "D" "E" "F"
sizes <- list_sizes(xs)
vec_partition(x, sizes)
Co-authored-by: Davis Vaughan <davis@rstudio.com>
Yeah, the failures seem to be cases that expected a single string. So there's always a trivial fix, which is to vectorise with |
Fixed in rmarkdown 2.17, but I think this will have to wait until the next release since otherwise installing stringr will break older (i.e. most) rmarkdown installed. |
#Conflicts: # NEWS.md # R/replace.R # man/str_replace.Rd # revdep/README.md # revdep/cran.md # revdep/problems.md # tests/testthat/_snaps/replace.md # tests/testthat/test-replace.R
I think enough time has elapsed that we can now merge this, fixing the 7 CRAN failures with PRs. |
This breaks 7 revdeps but I think it's worth it because the performance improvement is immense.