Skip to content
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

Closed
HughParsonage opened this issue May 24, 2018 · 7 comments · Fixed by #2205
Closed

Make stringr / stringi a soft dependency #1549

HughParsonage opened this issue May 24, 2018 · 7 comments · Fixed by #2205
Assignees
Labels
feature Feature requests

Comments

@HughParsonage
Copy link

HughParsonage commented May 24, 2018

knitr's dependency graph includes evaluate and stringr, and thus stringi. Package stringi 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. Since knitr (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 in evaluate. I'd keep the same verbs as stringr 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 for readr and stringi.

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.

@yihui
Copy link
Owner

yihui commented May 24, 2018

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!

HughParsonage added a commit to HughParsonage/evaluate that referenced this issue May 24, 2018
@jangorecki
Copy link

Hi guys, any progress on that issue? knitr is so crucial package that it should be as lightweight as possible. stringr is definitely first candidate to get rid off from mandatory deps.

@yihui
Copy link
Owner

yihui commented Jan 22, 2021

@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.

@cderv
Copy link
Collaborator

cderv commented Jan 26, 2021

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.

@cderv cderv added the feature Feature requests label Jan 27, 2021
@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

For reference, this could be of interest
https://github.com/hadley/stringb

@yihui yihui moved this to Backlog in R Markdown Team Projects Dec 21, 2021
@cderv cderv moved this from Backlog to To discuss / To plan in R Markdown Team Projects Sep 7, 2022
@cderv cderv removed their assignment Sep 13, 2022
@cderv cderv moved this from To discuss / To plan to Todo in R Markdown Team Projects Sep 13, 2022
@rich-iannone rich-iannone moved this from Todo to In Progress in R Markdown Team Projects Sep 20, 2022
@rich-iannone rich-iannone moved this from In Progress to Todo in R Markdown Team Projects Sep 20, 2022
@rich-iannone rich-iannone moved this from Next to Todo In Progress in R Markdown Team Projects Sep 22, 2022
@rich-iannone rich-iannone moved this from Todo In Progress to To discuss / To plan in R Markdown Team Projects Sep 22, 2022
yihui added a commit that referenced this issue Oct 19, 2022
yihui added a commit that referenced this issue Oct 19, 2022
@yihui yihui moved this from To discuss / To plan to Next in R Markdown Team Projects Oct 19, 2022
yihui added a commit that referenced this issue Nov 19, 2022
@yihui yihui moved this from Next to Todo In Progress in R Markdown Team Projects Dec 13, 2022
yihui added a commit that referenced this issue Dec 13, 2022
…2202, #1549)

* Replace str_detect() with grepl()

* Replace str_wrap() with base equivalent

* also get rid of str_pad()

* get rid of str_count()

Co-authored-by: Yihui Xie <[email protected]>
rich-iannone added a commit that referenced this issue Dec 22, 2022
* 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)
yihui added a commit that referenced this issue Dec 22, 2022
Repository owner moved this from Todo In Progress to Done in R Markdown Team Projects Dec 22, 2022
@yihui
Copy link
Owner

yihui commented Dec 22, 2022

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.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Feature requests
Projects
None yet
5 participants