You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/07/01 11:24:30 UTC
[GitHub] [arrow] thisisnic opened a new pull request #10638: ARROW-13171: [R] Add binding for str_pad()
thisisnic opened a new pull request #10638:
URL: https://github.com/apache/arrow/pull/10638
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r666191374
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,44 @@ test_that("str_like", {
df
)
})
+
+test_that("str_pad", {
+
Review comment:
Good point - have alternated between even and odd numbers for `width` now.
The second to last test uses a `width` value shorter than the input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook closed pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook closed pull request #10638:
URL: https://github.com/apache/arrow/pull/10638
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r664866847
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,44 @@ test_that("str_like", {
df
)
})
+
+test_that("str_pad", {
+
Review comment:
Also: should some of these tests use `width` that's less than the number of characters in the input? The `stringr::str_pad()` docs show an example like that:
```r
# Longer strings are returned unchanged
str_pad("hadley", 3)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r664864333
##########
File path: r/R/dplyr-functions.R
##########
@@ -414,6 +414,25 @@ nse_funcs$pmax <- function(..., na.rm = FALSE) {
)
}
+nse_funcs$str_pad <- function(string, width, side = c("left", "right", "both"), pad = " ") {
+
+ side <- match.arg(side)
Review comment:
Let's do some more argument validation to make better error messages if users pass unexpected inputs:
```suggestion
assert_that(is_integerish(width))
side <- match.arg(side)
assert_that(is.string(pad))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#issuecomment-872355697
Have now updated this PR to no longer skip a test, as the C++ code has changed and it now matches the R/stringr implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#issuecomment-872148421
Have opened a ticket for the centre padding functions as C++/R ways of doing spaces don't match and can't be configured to currently: https://issues.apache.org/jira/browse/ARROW-13234
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#issuecomment-872146860
https://issues.apache.org/jira/browse/ARROW-13171
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r664866847
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,44 @@ test_that("str_like", {
df
)
})
+
+test_that("str_pad", {
+
Review comment:
Also: should some of these tests use a `width` value that's less than the number of characters in the input? The `stringr::str_pad()` docs show an example like that:
```r
# Longer strings are returned unchanged
str_pad("hadley", 3)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r664867533
##########
File path: r/R/dplyr-functions.R
##########
@@ -414,6 +414,25 @@ nse_funcs$pmax <- function(..., na.rm = FALSE) {
)
}
+nse_funcs$str_pad <- function(string, width, side = c("left", "right", "both"), pad = " ") {
+
+ side <- match.arg(side)
Review comment:
I think this is important because `stringr::str_pad()` can take vector `width` and `pad` and recycle them, but that will not work here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r664867533
##########
File path: r/R/dplyr-functions.R
##########
@@ -414,6 +414,25 @@ nse_funcs$pmax <- function(..., na.rm = FALSE) {
)
}
+nse_funcs$str_pad <- function(string, width, side = c("left", "right", "both"), pad = " ") {
+
+ side <- match.arg(side)
Review comment:
I think this is important because `stringr::str_pad()` can take vector `width` and `pad` and recycle them along the length of the `string` vector, but that will not work here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#discussion_r664865288
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,44 @@ test_that("str_like", {
df
)
})
+
+test_that("str_pad", {
+
Review comment:
Should some of these tests use odd number `width`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#issuecomment-872231928
> See #10639. Sorry, I thought I had checked against the Python behavior at least, apparently not.
Wow, that was fast, thanks! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10638: ARROW-13171: [R] Add binding for str_pad()
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10638:
URL: https://github.com/apache/arrow/pull/10638#issuecomment-872198559
See #10639. Sorry, I thought I had checked against the Python behavior at least, apparently not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org