-
-
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
Closes #1549: weaken knitr's dependency on stringr #1552
Conversation
|
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.
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!
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. |
Okay. Great! I'll let you know when I actually start working on it (or have someone else work on it). Thanks again! |
This is the proposal to weaken knitr's dependency on
stringr
and close #1549. We replace eachstringr::<function>
withstringr__<function>
which is defined in0-stringr_suggestions.R
. Each such function reverts to the originalstringr
version ifuse_stringr()
isTRUE
, 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 variableKNITR_USE_STRINGR
and a package optionknitr.use.stringr
, both of which are by defaultTRUE
. The idea would to change the default toFALSE
in two releases' time so thatstringr
can be moved toSuggests
.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.all_patterns$md$inline.code
had to be changed. If users have used custom patterns that were valid instringi
but not base R, this might be difficult to detect when the time comes to detachstringr
.I believe this satisfies the integration tests: the failure seems to be a package caching issue.
stringr::str_sub<-
had slightly different behaviour tosubstr<-
(andstringi::str_sub
) so I had to write a specific function for the assignment.