-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add ignore_cache to fetch_args_list #301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I'm getting some tests failing when running locally where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor suggestions that probably don't relate to the refactor itself but also include some similar cleanup, but I did not spot why my local tests might be failing; looked good to me but seems that I missed something (hence just commenting). Here's the output if helpful:
ℹ Testing epidatr
! epidatr cache is being used (set env var EPIDATR_USE_CACHE=FALSE if not intended).
ℹ The cache directory is ~/.cache/R/epidatr.
ℹ The cache will be cleared after 14 days and will be pruned if it exceeds 4096 MB.
ℹ The log of cache transactions is stored at ~/.cache/R/epidatr/logfile.txt.
✔ | F W S OK | Context
✔ | 1 | auth
✖ | 1 42 | cache
──────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-cache.R:128:7): check_is_cachable
check_is_cachable(epidata_call, fetch_args) is not FALSE
`actual`: TRUE
`expected`: FALSE
Backtrace:
▆
1. └─epidatr (local) check_fun(...) at test-cache.R:142:3
2. └─testthat::expect_false(check_is_cachable(epidata_call, fetch_args)) at test-cache.R:128:7
──────────────────────────────────────────────────────────────────────────────────────────────
✔ | 19 | check
✔ | 3 | covidcast [1.2s]
✔ | 1 74 | endpoints
✔ | 12 | epidatacall
✔ | 6 | epirange
✔ | 42 | model
✔ | 1 0 | request
✔ | 1 28 | utils
══ Results ═══════════════════════════════════════════════════════════════════════════════════
Duration: 3.2 s
── Skipped tests (3) ─────────────────────────────────────────────────────────────────────────
• empty test (2): test-endpoints.R:1:1, test-utils.R:24:1
• This site is down. (1): test-request.R:2:3
── Failed tests ──────────────────────────────────────────────────────────────────────────────
Failure (test-cache.R:128:7): check_is_cachable
check_is_cachable(epidata_call, fetch_args) is not FALSE
`actual`: TRUE
`expected`: FALSE
Backtrace:
▆
1. └─epidatr (local) check_fun(...) at test-cache.R:142:3
2. └─testthat::expect_false(check_is_cachable(epidata_call, fetch_args)) at test-cache.R:128:7
[ FAIL 1 | WARN 0 | SKIP 3 | PASS 227 ]
Fixed a bunch of issues:
Thanks for the review! |
Sorry, somehow thought this was already merged. Taking a look now. Quick first not: I think running tests deleted my actual cache instead of the test cache somehow? And then I get output like:
from trying the Though some other attempts are easier to interpret:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catches. I've noted a couple minor issues + request (maybe in some separate Issue/PR) to improve messaging when a cache has been cleared unexpectedly and we still try to use it.
Also, I'm now thinking that |
* fix: only_supports_classic was parsed incorrectly * fix: clear_cache reuses previous set_cache settings * feat: add is_cache_enabled() * refactor: cache_epidata_call integrated into fetch() * refactor: removed fetch_tbl() and integrated into fetch() * test: fix cache tests to not clear user cache * ci: update old actions * doc: comment cacheable functions
Checklist
Please:
PR).
brookslogan, dshemetov, nmdefries, dsweber2.
DESCRIPTION
. Always incrementthe patch version number (the third number), unless you are making a
release PR from dev to main, in which case increment the minor version
number (the second number).
(backwards-incompatible changes to the documented interface) are noted.
Collect the changes under the next release number (e.g. if you are on
1.7.2, then write your changes under the 1.8 heading).
Change explanations for reviewer
ignore_cache
argument tofetch_args_list()
to make it easier to temporarily ignore the cacheis_cachable
property getter for claritycache_epidata_call
into pieces, move most of the pieces over tofetch
fetch_tbl
and simplifyfetch
logicMagic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch
fetch
call chain is too complex #200