Update check_ to initialise_dataclass

epiforecasts / covidregionaldata

An interface to subnational and national level COVID-19 data. For all countries supported, this includes a daily time-series of cases. Wherever available we also provide data on deaths, hospitalisations, and tests. National level data is also supported using a range of data sources as well as linelist data and links to intervention data sets.

https://epiforecasts.io/covidregionaldata/

Other

37 stars 18 forks source link

Update check_ to initialise_dataclass #291

Closed seabbs closed 3 years ago

seabbs commented 3 years ago

This PR unhelpfully combines some small documentation fixes and renaming the check_ function to a name better suited to both user use (as it seems like a useful function if not wanting to use the get_ wrappers). I have completed the PR checklist as requested.

github-actions[bot] commented 3 years ago

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

[x] Read our contribution guidelines if you have not already done so.
- [x] 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.
- [x] If you have added a new data class please run the tests locally for that class (using devtools::load_all(); devtools::test()).
- [x] Check your code passes our CI checks and review any style and code coverage warnings.
- [x] 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.

RichardMN commented 3 years ago

I was waiting for my output comparator to get somewhere and so couldn't build and pick through this this evening.

One thing which I had wondered which I thought might be addressed by this and could possibly wait for 0.9.1 is whether we should move the call to set_regions into download or even clean. Much of the rest of the initialize is fairly lightweight, but some of our set_regions code builds and rebuilds tibbles. I think I had thought it was only called once, and only called for the source which was being sought, but then I stepped through and realized that it seems to be called for every class the first time you ask what things are available.

seabbs commented 3 years ago

Ah interesting, yes my assumption had been that it was pretty light weight as well but perhaps it isn't...

If not in initialise I am not sure I like another location for it. It's not really downloading for example, and nor is it really cleaning (though perhaps this is more arguable). I guess you could have it as a standalone call but it really doesn't seem necessary to get the user to call it themselves.

For me I would vote leave as is for now but for sure open to doing something else targeting 0.9.1? The region codes in general seem really tricky to handle so I'd like to make it better but I am not clear what an "optimal" solution looks like.