diff --git a/.github/workflows/copilot-setup-steps.yml b/.github/workflows/copilot-setup-steps.yml new file mode 100644 index 0000000..58aab2a --- /dev/null +++ b/.github/workflows/copilot-setup-steps.yml @@ -0,0 +1,41 @@ +name: "Copilot Setup Steps" + +# Automatically run the setup steps when they are changed to allow for easy validation, and +# allow manual testing through the repository's "Actions" tab +on: + workflow_dispatch: + push: + paths: + - .github/workflows/copilot-setup-steps.yml + pull_request: + paths: + - .github/workflows/copilot-setup-steps.yml + +jobs: + # The job MUST be called `copilot-setup-steps` or it will not be picked up by Copilot. + copilot-setup-steps: + runs-on: ubuntu-latest + + permissions: + contents: read + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up R + uses: r-lib/actions/setup-r@v2 + with: + r-version: "release" + use-public-rspm: true + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.x" + + - name: Install R dependencies + uses: r-lib/actions/setup-r-dependencies@v2 + with: + extra-packages: any::rcmdcheck, any::devtools + needs: check diff --git a/NEWS.md b/NEWS.md index e9862e6..93992f7 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,7 @@ * Fixed a data conversion issue in `time_to_sec()` (#290). * Updated testing to comply with changes in `ggplot2` (#292). * Updated data conversions to avoid bugs after changes in Jolpica database. (#281, #284, #298, #299) +* Fixed a bug where a NULL error return from Jolpica would be cached, preventing retries when the API recovers. (#295) # f1dataR 2.0.1 diff --git a/R/zzz.R b/R/zzz.R index 3990c28..d8621b8 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -1,3 +1,26 @@ +# Wraps a cachem cache so that NULL function results are never stored. +# memoise stores results via withVisible(), so a NULL return is represented +# as list(value = NULL, visible = ...). By skipping set() for those values +# the cache misses on the next call, causing the underlying function to be +# retried rather than returning the cached NULL error. +null_filtering_cache <- function(cache) { + list( + get = function(key, ...) cache$get(key, ...), + set = function(key, value) { + if (is.list(value) && is.null(value$value)) { + return(invisible(NULL)) + } + invisible(cache$set(key, value)) + }, + exists = function(key, ...) cache$exists(key, ...), + remove = function(key, ...) cache$remove(key, ...), + reset = function(...) cache$reset(...), + keys = function(...) cache$keys(...), + info = function(...) cache$info(...), + prune = function(...) cache$prune(...) + ) +} + # nocov start .onLoad <- function(libname, pkgname) { reticulate::py_require("fastf1") @@ -28,9 +51,9 @@ # set the cachedir to our new location for fastf1 caching too options("f1dataR.cache" = cache_dir) } - cache <- cachem::cache_disk(dir = memoise_option) + cache <- null_filtering_cache(cachem::cache_disk(dir = memoise_option)) } else if (memoise_option == "memory") { - cache <- cachem::cache_mem() + cache <- null_filtering_cache(cachem::cache_mem()) } if (memoise_option != "off") { diff --git a/tests/testthat/test-load_laps.R b/tests/testthat/test-load_laps.R index be18454..aa13d72 100644 --- a/tests/testthat/test-load_laps.R +++ b/tests/testthat/test-load_laps.R @@ -56,5 +56,9 @@ test_that("load_laps works without internet", { }) }) }) + + # NULL (error) results must not be stored in the cache so that subsequent + # calls retry the request rather than returning the cached failure. + expect_false(memoise::has_cache(load_laps)(2021, 1)) } }) diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index df7c433..b9348b2 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -49,6 +49,30 @@ test_that("utility functions work", { ) }) +test_that("null_filtering_cache prevents NULL results from being stored", { + inner <- cachem::cache_mem() + filtered <- f1dataR:::null_filtering_cache(inner) + + # Non-NULL values must be stored and retrievable + filtered$set("k1", list(value = "hello", visible = TRUE)) + expect_true(filtered$exists("k1")) + expect_equal(filtered$get("k1")$value, "hello") + + # NULL values must NOT be stored + filtered$set("k2", list(value = NULL, visible = FALSE)) + expect_false(filtered$exists("k2")) + + # Other cache methods delegate correctly + filtered$remove("k1") + expect_false(filtered$exists("k1")) + + filtered$set("k3", list(value = 42L, visible = TRUE)) + expect_true(filtered$exists("k3")) + filtered$reset() + expect_false(filtered$exists("k3")) +}) + + test_that("Utility Functions work without internet", { # Set testing specific parameters - this disposes after the test finishes if (dir.exists(file.path(tempdir(), "tst_utils2"))) {