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

Closes #1549: weaken knitr's dependency on stringr #1552

Closed
wants to merge 63 commits into from

Conversation

HughParsonage
Copy link

@HughParsonage HughParsonage commented Jun 5, 2018

This is the proposal to weaken knitr's dependency on stringr and close #1549. We replace each stringr::<function> with stringr__<function> which is defined in 0-stringr_suggestions.R. Each such function reverts to the original stringr version if use_stringr() is TRUE, and a base R version otherwise.

How exactly the proposal should proceed in a release is probably something you are better placed to decide. As it is, knitr would introduce an environment variable KNITR_USE_STRINGR and a package option knitr.use.stringr, both of which are by default TRUE. The idea would to change the default to FALSE in two releases' time so that stringr can be moved to Suggests.

There are a couple of changes:

  • str_wrap could not be faithfully replicated using base R functions, though the basic element of it has. I could probably try very hard to recreate it exactly, but I would probably not do it exactly. It seemed to be less part of the 'core' of knitr, but I may have misinterpreted its importance.
  • Some regular expressions which were valid in stringi were not valid in base R, notably variable-length lookbehinds. In particular, the pattern all_patterns$md$inline.code had to be changed. If users have used custom patterns that were valid in stringi but not base R, this might be difficult to detect when the time comes to detach stringr.

I believe this satisfies the integration tests: the failure seems to be a package caching issue.

stringr::str_sub<- had slightly different behaviour to substr<- (and stringi::str_sub) so I had to write a specific function for the assignment.

@yihui yihui changed the title Closes #1549 Closes #1549: weaken knitr's dependency on stringr Nov 7, 2018
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not having looked at this PR earlier. Since it has been a long while, do you prefer me to carry it on, or you to continue working on it?

Actually I'd love to get rid of one stringr function a time in a separate PR. Trying to get rid of all of them in one giant PR may be impractical and also makes it more difficult to review/test.

Thank you so much!

@HughParsonage
Copy link
Author

Happy either way. My guess is that it’s easier for you to take what I’ve got, create separate PRs and ask questions (feel free to tag me to clarify anything). Most of the trickiness will be in the use_stringr ui which you are probably better placed to assess.

@yihui
Copy link
Owner

yihui commented Sep 30, 2020

Okay. Great! I'll let you know when I actually start working on it (or have someone else work on it). Thanks again!

@crsh crsh mentioned this pull request Oct 15, 2021
3 tasks
@yihui yihui closed this in cc3b92a Dec 22, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make stringr / stringi a soft dependency
3 participants