Skip to content
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

Update cache management #228

Merged
merged 13 commits into from
Sep 30, 2021
Merged

Update cache management #228

merged 13 commits into from
Sep 30, 2021

Conversation

dieghernan
Copy link
Member

Closes #223

See here an implementation of the new cache management. There is a suite of functions that detects the current cache dir (either set up via parameter, via options(eurostat_cache_dir or installed in the machine) and manages the download.

A similar suite is implemented in two of my packages, already on CRAN: https://github.com/rOpenGov/giscoR and https://github.com/rOpenSpain/mapSpain

The current approach only writes on the temporary directory unless the user set a cache dir. Also, for storing the persistent cache dir with rappdirs the user needs to explicitly install the cache dir path with install = TRUE.

I updated also some links, the cache parts on the functions that uses it and the clean_eurostat_cache function.

There is also a whole test suite for that. Note that the test suite also switches temporarly the cache dir to run the tests on a temporary dir, so no files are downloaded or removed on the local machine.

By last, I updated the GH check action to cache dependencies also for Windows (would reduce significantly the running time) and for showing the tests output.

@dieghernan dieghernan marked this pull request as draft September 29, 2021 13:50
@dieghernan dieghernan marked this pull request as ready for review September 29, 2021 13:59
Copy link
Member

@antagomir antagomir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good! Shall we merge or are more commits still flowing in?

@pitkant
Copy link
Member

pitkant commented Sep 29, 2021

Seems good. Some notes:

@dieghernan
Copy link
Member Author

Hi! I’ll tackle your comments and I would let you know, thanks both

@dieghernan
Copy link
Member Author

It’s done now, moving to testthat 3 made me change some tests due to deprecations (a bit of pain).

I’ll leave it to you if finally you decided to merge @antagomir @pitkant

@antagomir
Copy link
Member

Ok to me, how about @pitkant

Conflicts:
	DESCRIPTION
Copy link
Member

@pitkant pitkant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hard work, looks great!

@dieghernan dieghernan merged commit a98e3ad into rOpenGov:master Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential improvement on cache dir
3 participants