-
-
Notifications
You must be signed in to change notification settings - Fork 872
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
Make stringr / stringi a soft dependency #1549
Comments
Thanks for asking first! I'd love to get rid of the stringr/stringi dependencies if possible. Installing stringi also bothers myself a bit. Please feel free to work on it and I'll be happy to review the PR. Thanks! |
Hi guys, any progress on that issue? knitr is so crucial package that it should be as lightweight as possible. |
@jangorecki Sorry, but no yet. As I said above, I'd love to get rid of the stringr dependency. @HughParsonage has kindly contributed a big PR #1549 that I haven't had time to digest yet. I feel it may be easier to achieve the goal if we replace one stringr function at one step (i.e., replace each function in a separate PR). We could start with the easier ones, and decide what to do with the hard bones later. |
For later reference, knitr is currently using 13 functions for stringr that would need to be rewrite without stringr. library(dplyr)
itdepends::dep_usage_pkg("knitr") %>% filter(pkg == "stringr") %>% distinct()
#> Warning: `as.tibble()` is deprecated as of tibble 2.0.0.
#> Please use `as_tibble()` instead.
#> The signature and semantics have changed, see `?as_tibble`.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_warnings()` to see where this warning was generated.
#> # A tibble: 13 x 2
#> pkg fun
#> <chr> <chr>
#> 1 stringr str_detect
#> 2 stringr str_count
#> 3 stringr str_locate
#> 4 stringr str_sub
#> 5 stringr str_locate_all
#> 6 stringr str_extract_all
#> 7 stringr str_wrap
#> 8 stringr str_trim
#> 9 stringr str_pad
#> 10 stringr str_dup
#> 11 stringr str_split
#> 12 stringr str_match_all
#> 13 stringr str_replace_all One thing to watch out for is the performance if we remove stringi as we don't want to lose performance. |
For reference, this could be of interest |
Co-authored-by: Yihui Xie <xie@yihui.name>
…, #1549) Co-authored-by: Yihui Xie <xie@yihui.name>
* master: make negative times 0 replace highr:::spaces() with strrep() in base R support chunk options message/warning = NA close #2204: drop the support for cairoDevice amend 1a0f2cc: make line_count() a few times faster Replace several stringr-based function calls with base equivalents (#2202, #1549) see if this fix the ruby gems issue https://github.com/yihui/knitr/actions/runs/3682745993/jobs/6230638404 stop importing xfun::isFALSE() and define isFALSE() only for R < 3.5.0 Add labels to the default progress bar and allow users to provide a custom progress bar (#2196)
We have finally finished removing stringr from the dependency. We will do more testing and also benchmarking later. Everyone is welcome to help us test and benchmark the dev version! Thanks! P.S. The benchmark should be done against knitr v1.40 but we did the stringr removal in several commits since September (9c92eff 8fa7d17 1ce8286 67b973d 5a2cb72 1a0f2cc cc3b92a b1ce0c7), so benchmarking can be tricky. |
This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary. |
knitr
's dependency graph includesevaluate
andstringr
, and thusstringi
. Packagestringi
is a heavy dependency: it takes around a minute to install from source (or several on OSX or Windows) -- a typical use-case for things like Travis-CI. Sinceknitr
(to its immense credit) is required for testing by an enormous number of packages, shaving a minute from each test could have a large payoff. (I also use Travis-CI to deploy knitr+LaTeX documents and installing stringi is a significant component of the build time.)I'd propose to move
stringr
to Suggests both here and inevaluate
. I'd keep the same verbs asstringr
uses, but switch to base R if stringi is not installed. See https://github.com/HughParsonage/TeXCheckR/blob/master/R/suggested_speedups.R for an example of softening the dependencies forreadr
andstringi
.A glance at the parts of the source currently importing stringr makes me think this would not be too hard. I'd be happy to do the work, but your contribution guidelines say to raise an issue before starting work on something like this.
The text was updated successfully, but these errors were encountered: