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

Replace stringr functions with base R equivalents #1043

Merged
merged 14 commits into from
Sep 7, 2022
Merged

Conversation

rich-iannone
Copy link
Member

@rich-iannone rich-iannone commented Sep 7, 2022

This PR removes the few stringr functions that remained in the package. A handful of simple replacement functions are used instead.

@rich-iannone rich-iannone marked this pull request as ready for review September 7, 2022 12:07
@rich-iannone rich-iannone requested a review from cscheid September 7, 2022 12:07
@cderv
Copy link
Collaborator

cderv commented Sep 7, 2022

@rich-iannone Interested by this work for knitr too: yihui/knitr#1549

I don't know if all the functions are covered by your switch here or not, but definitely some overlap.

Only constraint for knitr - performance. We need to be sure that we don't loose performance in the rendering process. Maybe also an issue with gt

@cscheid cscheid merged commit d3d590c into master Sep 7, 2022
@rich-iannone rich-iannone deleted the stringr-replace branch September 7, 2022 16:18
@rich-iannone
Copy link
Member Author

@rich-iannone Interested by this work for knitr too: yihui/knitr#1549

I don't know if all the functions are covered by your switch here or not, but definitely some overlap.

Only constraint for knitr - performance. We need to be sure that we don't loose performance in the rendering process. Maybe also an issue with gt

@cderv Many of the stringr functions are covered here by the replacements. I could definitely do the same for knitr, not a problem. A lot of the replacements in this PR are concentrated in one part of the code path (inlining styles in HTML tags). It's not any slower and that whole section will be replaced soon anyway with a fast JS-mediated function.

@cderv
Copy link
Collaborator

cderv commented Sep 7, 2022

Many of the stringr functions are covered here by the replacements. I could definitely do the same for knitr, not a problem.

Awesome !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants