vubiostat / redcapAPI

R interface to REDCap (http://www.project-redcap.org/)
16 stars 26 forks source link

Date Validation (FIXME) #369

Closed spgarbet closed 2 weeks ago

spgarbet commented 1 month ago

At present a simple working date, date time, and date time seconds validations are included. They don't validate that the date specified is a an actual date, for example 2003-04-31 is accepted by the routines. A more complex regex is possible, but this would still allow any Feb 29th ignoring the leap year computation. Is there a way to validate a date quickly pre-cast that could be put into the existing validations.

https://github.com/vubiostat/redcapAPI/blob/main/tests/testthat/test-053-fieldValidationAndCasting.R#L23

spgarbet commented 1 month ago

We have a lubridate dependency already.

> ymd(c('2024-02-31', '2024-04-31', '2024-05-26'))
[1] NA           NA           "2024-05-26"

This validates dates. Maybe we should just do this and be done.

nutterb commented 1 month ago

The lubridate dependency exists for older code that is scheduled for removal with 3.0.0. The 3.0.0 branch doesn't have this dependency.

That doesn't mean we can't put it back in. Just pointing out that we had made deliberate steps to remove it and so I'm not sure "we already have the dependency" is the right justification. It should really be a question of whether carrying the dependency carries enough benefit.

nutterb commented 1 month ago

And, for what it's worth, the base R functions return something similar

> x <- c('2024-02-31', '2024-04-31', '2024-05-26')
> 
> as.Date(x, format = "%Y-%m-%d")
[1] NA           NA           "2024-05-26"
> 
> as.POSIXct(x, format = "%Y-%m-%d")
[1] NA               NA               "2024-05-26 EDT"
spgarbet commented 1 month ago

Yes, we had that conversation about 3.0.0 before. I forgot about these things.

The issue that spawned exportRecordsTyped to begin with was

> as.Date("sdfasdf")
Error in charToDate(x) : 
  character string is not in a standard unambiguous format

The validation is preventing this from crashing. Two approaches then open themselves up:

(1) Validating that it won't crash on type casting is enough. Stick with what we have. Delete the FIXME note and move on. (2) Use the regex as the first pass, then use the as.Date as the second pass for validation. Pros: Utterly correct. Cons: as.Date conversions happen twice.

As a bonus I can probably get rid of the lubridate code pretty easily. (and with 3.0.0 that whole routine goes away).

spgarbet commented 2 weeks ago

Reviewed with Cole. Here's the plan:

(1) Delete FIXME and leave defaults as is. (2) Create new method that a user can specify for validation: strictDateValidate, it uses the double call. (3) Update documentation to point out the possible choice of validation to the user.

All in all, the strategy is to use the interface as designed and provide choices to the user.