stringr icon indicating copy to clipboard operation
stringr copied to clipboard

Vectorise replacement function

Open hadley opened this issue 3 years ago • 5 comments

This breaks 7 revdeps but I think it's worth it because the performance improvement is immense.

hadley avatar Oct 04 '22 12:10 hadley

It also breaks rmarkdown 😬

Error in `if (grepl("^[.][.]", in_file)) ...`:
! the condition has length > 1
---
Backtrace:
     â–†
  1. └─pkgdown::build_article("from-base", new_process = FALSE)
  2.   └─pkgdown:::render_rmarkdown(...) at pkgdown/R/build-articles.R:281:2
  3.     ├─rlang::inject(rmarkdown::render(!!!args)) at pkgdown/R/rmarkdown.R:52:4
  4.     └─rmarkdown::render(...) at rlang/R/eval.R:195:2
  5.       └─output_format$post_processor(...)
  6.         └─rmarkdown (local) base(metadata, input_file, output_file, ...)
  7.           └─rmarkdown:::process_images(output_str, image_relative)
  8.             └─rmarkdown:::process_html_res(...)
  9.               └─stringr::str_replace_all(html, reg, process_img_src)
 10.                 └─stringr:::str_transform_all(string, pattern, replacement) at stringr/R/replace.r:91:4
 11.                   └─rmarkdown (local) replacement(old_flat) at stringr/R/replace.r:201:4
 12.                     └─rmarkdown (local) processor(img_src, src)

hadley avatar Oct 04 '22 13:10 hadley

rmarkdown patch in https://github.com/rstudio/rmarkdown/pull/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.

hadley avatar Oct 04 '22 14:10 hadley

I guess pkgdown is failing because rmarkdown is broken? Very meta - ah yes i see that in your backtrace

DavisVaughan avatar Oct 04 '22 16:10 DavisVaughan

Yeah, the failures seem to be cases that expected a single string. So there's always a trivial fix, which is to vectorise with vapply() or similar. When I do the revdep fixes, I'll try and vectorise properly so they can benefit from the performance boost.

hadley avatar Oct 05 '22 11:10 hadley

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.

hadley avatar Oct 20 '22 13:10 hadley

I think enough time has elapsed that we can now merge this, fixing the 7 CRAN failures with PRs.

hadley avatar Jul 16 '24 12:07 hadley