Skip to content

refactor: remove Suggests dependence on covidcast #536

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

Merged
merged 9 commits into from
Oct 4, 2024
Merged

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Oct 3, 2024

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • Request a review from one of the current main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION. Always increment
    the 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).
  • Describe changes made in NEWS.md, making sure breaking changes
    (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).
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

  • just download the files, don't import covidcast
  • covidcast depends on sf which is tough to install (it just refuses to compile on my machine) and my renv keeps trying to install ALL dependencies

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

  • Resolves #{issue number}

* just download the files, don't import covidcast
* covidcast depends on sf which is tough to install
@dshemetov dshemetov requested a review from brookslogan October 3, 2024 02:16
@brookslogan
Copy link
Contributor

I think we can do this more simply with a small addition to #520. Reviewing #520 now and going to try out.

@dshemetov
Copy link
Contributor Author

Makes sense. Should probably be solved upstream in this case cmu-delphi/epidatasets#7

@brookslogan
Copy link
Contributor

brookslogan commented Oct 3, 2024

state_census at least is already there... but I guess county_census is not. Unfortunate, would require upstream work I guess. Hoped it was a little bit simpler.

@dshemetov
Copy link
Contributor Author

It's basically a one line change to the epidatasets scripts, it's not bad

* Rename state_census -> state_naming.
* Provide col_types specs for all & only columns used; avoid message spam.
* Don't select unused cols; especially avoid the numeric state FIPS.
* Bump dependency on dplyr and update joins; avoid message spam.
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Think this is good to go. Going to force-push to try to make some checks happen.

@brookslogan brookslogan merged commit 63cb820 into dev Oct 4, 2024
4 checks passed
@brookslogan brookslogan deleted the ds/file branch October 4, 2024 17:55
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.

2 participants