Closed bsweger closed 7 months ago
@bsweger I'm assuming you will ping me when this is ready for a full review, and am holding off on a review until then.
@nickreich ready for review!
A bigger point is that although the github action definition updates looked plausible to me, I don't know of a way to test that they work without actually merging the changes in and giving them a run... do you?
@elray1 r_cmd_check.yaml
and pr_unittest.yaml
run when a PR opens, so I made sure they worked before opening the PR for review (however, the tests started failing after I committed your suggested changes, so I'll look at that).
is pkgdown.yaml
something we can run on a PR and then again on merge to main?
I recall that github actions have some capacity for ad-hoc runs, but I haven't looked into it yet.
@elray1 I believe I found and fixed the issue that was causing the tests to fail (that said, I'm not sure why they were passing before merging in your requested changes (which seem unrelated..and I reverted them to just see if the tests would start passing again, and they did! :tableflip:)
Anyway, everything is green now, and I squashed the commits so the history is a bit cleaner. Can you re-review when you have a chance? Thanks!
resolves #404, resolves #405
The scope of these commits is making sure that all required dependencies are in the package's DESCRIPTION instead of being explicitly installed as a workflow step (for example, DT is required to build the vignettes).
Additionally, there's some cleanup to remove workflow-based installs if they aren't being used and/or reducing the footprint (e.g., explicitly use
ggplot2
in the vignette rather than pull in the entire tidyverse).Longer term, we have a goal to make these workflows better align to the newer ones used in the hubverse (#406 )
note: there are several smaller commits so it's easier to revert any breaking changes...happy to squash after approval to tidy up the commit history