jonkeane commented on a change in pull request #12429:
URL: https://github.com/apache/arrow/pull/12429#discussion_r807278266
##########
File path: r/R/dplyr-funcs-datetime.R
##########
@@ -147,5 +147,14 @@ register_bindings_datetime <- function() {
register_binding("pm", function(x) {
!call_binding("am", x)
})
-
+ register_binding("semester", function(x, with_year = FALSE) {
+ month <- call_binding("month", x)
+ semester <- call_binding("if_else", month <= 6, 1L, 2L)
+ if (with_year) {
+ year <- call_binding("year", x)
+ return(year + semester / 10)
Review comment:
😱 This looks totally wrong, but it isn't. It's exactly what lubridate's
semseter/quarter returns: a numeric where the decimal portion is the quarter or
semester number (and not a decimal representation of such, or a string).
##########
File path: r/tests/testthat/test-dplyr-funcs-datetime.R
##########
@@ -711,3 +711,19 @@ test_that("am/pm mirror lubridate", {
)
})
+
+test_that("semester", {
+ test_df <- tibble(
+ month = c(1:12, NA),
+ month_char_pad = ifelse(month < 10, paste0("0", month), month),
+ dates = as.Date(paste0("2021-", month_char_pad, "-15"))
Review comment:
```suggestion
month_as_int = c(1:12, NA),
month_as_char_pad = ifelse(month < 10, paste0("0", month), month),
dates = as.Date(paste0("2021-", month_char_pad, "-15"))
```
and then also test `month_as_int` and `month_as_str_pad` below too
##########
File path: r/tests/testthat/test-dplyr-funcs-datetime.R
##########
@@ -711,3 +711,19 @@ test_that("am/pm mirror lubridate", {
)
})
+
+test_that("semester", {
+ test_df <- tibble(
+ month = c(1:12, NA),
+ month_char_pad = ifelse(month < 10, paste0("0", month), month),
+ dates = as.Date(paste0("2021-", month_char_pad, "-15"))
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(sem_wo_year = semester(dates),
+ sem_w_year = semester(dates, with_year = TRUE)) %>%
+ collect(),
+ test_df
+ )
Review comment:
Would you mind also testing some bad / wrong inputs and confirming that
the errors are what we expect? Something like:
```
expect_error(call_binding("semester", "not a month"), "...")
```
And other inputs like `22`, `-4`, `NA`, etc. that are clearly out of bounds
/ won't coerce into a month? Alternatively (or even better...) might be to add
these tests to the tests for the `month` binding since that's where that
coercion happens, it will be used when we make a binding for `quarters` and it
appears those tests weren't added (or I can't find them!) when we added `months`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
