[GitHub] [arrow] jonkeane commented on a diff in pull request #13070: ARROW-15804: [R] Update as.Date() to support several tryFormats

[GitBox <gi...@apache.org>]

jonkeane commented on code in PR #13070:
URL: https://github.com/apache/arrow/pull/13070#discussion_r870332856


##########
r/tests/testthat/test-dplyr-funcs-datetime.R:
##########
@@ -1490,8 +1490,10 @@ test_that("`as.Date()` and `as_date()`", {
     dt_utc = ymd_hms("2010-08-03 00:50:50"),
     date_var = as.Date("2022-02-25"),
     difference_date = ymd_hms("2010-08-03 00:50:50", tz = "Pacific/Marquesas"),
-    character_ymd_var = "2022-02-25 00:00:01",
-    character_ydm_var = "2022/25/02 00:00:01",
+    character_ymd_hms_var = "2022-02-25 00:00:01",
+    character_ydm_hms_var = "2022/25/02 00:00:01",
+    character_ymd_var = "2022-02-25",
+    character_ydm_var = "2022/25/02",
     integer_var = 32L,

Review Comment:
   We should add at least one test with multiple dates (and especially one with multiple formats, like `c("2022-01-23", "2022/01/23")`) I suspect that will surface some interesting consequences for the operational parts of this code. For example:
   
   ```
   > as.Date(c("2022-01-01", "2022/01/01"), tryFormats = c("%Y-%m-%d", "%Y/%m/%d"))
   [1] "2022-01-01" NA     
   > lubridate::as_date(c("2022-01-01", "2022/01/01"))
   [1] "2022-01-01" "2022-01-01"
   ```
   
   We don't need to necessarily change this particular fixture (though having multiple rows is generally a good idea that we probably should have caught earlier...). But we should definitely have tests for this behavior.



-- 
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