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

Modify get available datasets #382

Merged
merged 6 commits into from
Jun 16, 2021
Merged

Modify get available datasets #382

merged 6 commits into from
Jun 16, 2021

Conversation

joseph-palmer
Copy link
Collaborator

Rather than initialising every class when calling get_available_datasets this PR moves the data produced by get_available_datasets to package data (contained in data/) called all_country_data (happy to change this name). Now calling get_available_datasets() returns the saved data (and also filters for type using the type argument). The previous version of get_available_data() is now in a new function called render_available_data() which, when called will create the table by initalising each class like the previous version did.

A new R file called render_available_datasets.R in data-raw/ writes the all_country_data to the package data (which is also defined in datasets.R). To ensure contributors run this after adding a new dataset class I have added a test which will compare the current available datasets with that outputed by running render_available_datasets(). This will fail if a dataset class is added or if something in the class which is used in the table is modified (such as number of levels or urls) but the all_country_data table has not be re-rendered by running the render_available_datasets.R script. If this fails it will highlight this and suggest to run render_available_datasets.R.

closes #372

@github-actions
Copy link

github-actions bot commented Jun 15, 2021

👋 Thanks for opening this pull request! Can you please run through the following checklist before requesting review (ticking as complete or if not relevant).

  • Read our contribution guidelines if you have not already done so.
  • If you have altered an existing class please run the tests locally (using devtools::load_all(); devtools::test()) first setting options(testDownload=TRUE, testSource=class-name) and report your findings.
  • If you have added a new data class please run the tests locally for that class (using devtools::load_all(); devtools::test()).
  • Check your code passes our CI checks and review any style and code coverage warnings.
  • Comment with details if unable to get our CI checks to pass or unable to remove all warnings.

Thank you again for the contribution. If making large scale changes consider using our pre-commit hooks (see the contributing guide) to more easily comply with our guidelines.

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Nice idea. Maybe could do both of this in one function with an arg?

data-raw/render_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
…sets with options for rendering. also added parameter for namespace and updated tests
@joseph-palmer joseph-palmer requested a review from seabbs June 16, 2021 06:15
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Looks good. Just the bells and whistles to bring into line.

R/datasets.R Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Outdated Show resolved Hide resolved
R/get_available_datasets.R Show resolved Hide resolved
data-raw/render_available_datasets.R Outdated Show resolved Hide resolved
tests/testthat/test-get_available_datasets.R Outdated Show resolved Hide resolved
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Looks good to me. This feels quite elegant.

@seabbs seabbs merged commit f952cfc into master Jun 16, 2021
@seabbs seabbs deleted the modify-get_available_datasets branch June 16, 2021 15:06
@seabbs
Copy link
Contributor

seabbs commented Jun 16, 2021

ah did we include this in the news and increment the dev version?

@joseph-palmer
Copy link
Collaborator Author

Ah no I forgot, sorry!

@seabbs
Copy link
Contributor

seabbs commented Jun 16, 2021

No problem we can slip it in as a news update prior to release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling get_available_datasets initialises a copy of every class object - this may cause unexpected problems
2 participants