Skip to content

ARROW-11513: [R] Bindings for sub/gsub#9878

Closed
ianmcook wants to merge 19 commits intoapache:masterfrom
ianmcook:ARROW-11513
Closed

ARROW-11513: [R] Bindings for sub/gsub#9878
ianmcook wants to merge 19 commits intoapache:masterfrom
ianmcook:ARROW-11513

Conversation

@ianmcook
Copy link
Copy Markdown
Member

@ianmcook ianmcook commented Apr 2, 2021

Also stringr::str_replace() and stringr::str_replace_all()

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 2, 2021

Comment thread r/R/dplyr.R
Comment thread r/tests/testthat/test-dplyr-string-functions.R Outdated
Comment thread r/tests/testthat/test-dplyr-string-functions.R Outdated
Comment thread r/tests/testthat/test-dplyr-string-functions.R
@ianmcook ianmcook changed the title ARROW-11513: [R] Bindings for sub/gsub [WIP] ARROW-11513: [R] Bindings for sub/gsub Apr 5, 2021
@ianmcook ianmcook marked this pull request as ready for review April 5, 2021 19:34
Comment thread r/R/dplyr.R
Comment on lines +443 to +451
if (ignore.case) {
if (fixed) {
pattern <- gsub("\\E", "\\e", pattern, fixed = TRUE)
pattern <- paste0("(?i)\\Q", pattern, "\\E")
replacement <- gsub("\\", "\\\\", replacement, fixed = TRUE)
} else {
pattern <- paste0("(?i)", pattern)
}
}
Copy link
Copy Markdown
Member Author

@ianmcook ianmcook Apr 5, 2021

Choose a reason for hiding this comment

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

@maartenbreddels since ARROW-10306 did not directly support case-insensitive fixed string replacement (non-regex), this is the workaround we used to support this in the R bindings. Feedback welcome, and thanks for your work on the underlying C++ code!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comments added explaining this code in db828be

Copy link
Copy Markdown
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

Looking close, just a few more questions

Comment thread r/tests/testthat/test-dplyr-string-functions.R Outdated
Comment thread r/tests/testthat/test-dplyr-string-functions.R
Comment thread r/tests/testthat/test-dplyr-string-functions.R Outdated
Comment thread r/tests/testthat/test-dplyr-string-functions.R Outdated
Comment thread r/R/dplyr.R
Comment thread r/R/dplyr.R
Comment thread r/tests/testthat/test-dplyr-string-functions.R
Comment thread r/tests/testthat/test-dplyr-string-functions.R Outdated
Copy link
Copy Markdown
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

+1, nice work! Will merge when CI is green-ish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants