Issue 389: minimum supported R version 3.5 or 4.1 #402
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #402 +/- ##
=======================================
Coverage 96.26% 96.27%
=======================================
Files 15 15
Lines 1903 1905 +2
=======================================
+ Hits 1832 1834 +2
Misses 71 71 ☔ View full report in Codecov by Sentry. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f448548 is merged into main:
|
lintr is disabled in |
works for me - can't imagine there's so much legitimate false positive in |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f448548 is merged into main:
|
|
Noted in conversation w/ @seabbs: should this work also include a specific check that |
|
If we wanted to do this it would involve adding a matrix component for R 3.5 here https://github.com/epinowcast/epinowcast/blob/main/.github/workflows/R-CMD-check.yaml |
|
Good point. Perhaps just on Ubuntu? Or do we want the CI to include OSX? |
Yeah I think so. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b80247f is merged into main:
|
|
@pearsonca @seabbs good call as always on the addition of R3.5 to the test suite (I was holding my breathe with the GHA flow control). Looks like I think R3.6 is what ships with most Linux distributions as default now, so the bump to 3.6 as a minimum might be acceptable to most users. What do y'all think? |
|
3.6, aye. I supposed the lifecycle hard dependency is on changes to how packages were managed in 3.5 => 3.6? Dunno. |
There was a problem hiding this comment.
Looks good to me once we bump up to 3.6 and add a news item. Nice one @medewitt / @pearsonca
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b80247f is merged into main:
|
|
Whelp... found a test with the new syntax I missed and have an esoteric error on date conversions that I will hunt down. |
|
Found the weird date issue. In R 4.1 if you pass a NULL to epinowcast::coerce_date(NULL)
#> IDate of length 0
In R 3.6.1 you get an error: epinowcast::coerce_date(NULL)
#>Error in as.Date.default(x, tz = tz, ...) :
#> do not know how to convert 'x' to class “Date”Following the stacktrace, the failure was in the base # R4.1 source
as.Date.default <-
function (x, ...)
{
if (inherits(x, "Date"))
x
else if (is.null(x)) # <- this is the important bit...
.Date(numeric())
else if (is.logical(x) && all(is.na(x)))
.Date(as.numeric(x))
else stop(gettextf("do not know how to convert '%s' to class %s",
deparse1(substitute(x)), dQuote("Date")), domain = NA)
}
# R 3.6.1
as.Date.default <-
function (x, ...)
{
if (inherits(x, "Date"))
x
else if (is.logical(x) && all(is.na(x)))
.Date(as.numeric(x))
else stop(gettextf("do not know how to convert '%s' to class %s",
deparse(substitute(x)), dQuote("Date")), domain = NA)
}However |
|
Well, I don't have a good alternative at the moment. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b80247f is merged into main:
|
Hmm. I think we should prefer to have unit testing only require the same environment as the package requires - while possible to ensure that it's a real test for the minimal supported version of R, seems 1) time consuming to ensure and 2) still error prone. However, as a last resort, the tests are not what we provide the user. @Bisaloo @seabbs Does it work for that script to have an |
...also, is everything great? Do we want a NULL here to pass or to error? I'd have to look a bit closely at the context, but the rule of thumb is passing something a NULL indicates trying to do something that doesn't make sense (or at least warrants a warn/message). |
Yes, this sounds good to me 👍 |
Right now we have holidays set to NULL in the Line 64 in b80247f A guess a couple of actions we could do:
I think (3) and (4) we should probably do, and maybe (2). What do you think? |
|
TIL |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b80247f is merged into main:
|
|
Wow this is turning into an iceberg...
It sounds like this might have to be the minimum version then unless we want to experience some real pain reverting these (and having a less clean code base). It sounds like we probably want to do all of 1-4 as long as that doesn't result in all the default models throwing warnings (as not passing holidays is likely the default behaviour of most users). |
|
Given all this (sorry!) I am wondering if we just want to have 4.1 as our minimum version and make sure to test that going forward? |
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 473ab89 is merged into main:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if fda8ddb is merged into main:
|
|
Looking at the code coverage block, should be easy enough to address adding a copy-paste of the If you do so, eliminating the check null vs check empty branch distinction seems worthwhile to improve the static analysis. The tests set the contract, not the logic flow itself. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if fda8ddb is merged into main:
|
|
@pearsonca do you have time to give this a final review? I think its good t ogo. |
Description
This PR closes #389 (maybe...).
Replaces post R4.1.0 anonymous function syntax (
\(x)) and native pipe (|>) from the source and test files.Does not remove or notify the users that R >= 4.1.0 is necessary for running or rebuilding the vignettes.
This will at least guarantee that the package will build and test on platforms with R >= 3.5.
Checklist