sportsdataverse / wehoop

An R package to quickly obtain clean and tidy women's basketball play by play data.
https://wehoop.sportsdataverse.org/
Other
17 stars 2 forks source link

Expect a `"notes"` column in the scoreboard functions #15

Closed DavisVaughan closed 2 years ago

DavisVaughan commented 2 years ago

Followup to #14

The other change that comes with https://github.com/tidyverse/tidyr/pull/1200 is that now list-columns that contained completely empty cells are correctly retained. When you downloaded scoreboard data, there was a $notes element that always held list(). That column is now retained in the dev version of tidyr, so a few tests need to be updated.

Luckily, since those tests aren't run on CRAN, you won't get flagged by CRAN when we release tidyr.

Ideally, you'd merge #14 for us and send a new version of your package to CRAN, then we'd release tidyr, then you could merge in this PR and require the new version of tidyr.

This PR won't pass GitHub Action checks, because it needs #14.

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/saiemgilani/wehoop/GrJcvN6CNFi9cY2izAAGNRvtpKHd
✅ Preview: https://wehoop-git-fork-davisvaughan-fix-dev-tidyr-n-8b7bc0-saiemgilani.vercel.app

saiemgilani commented 2 years ago

That notes column is on occasion important otherwise I'd drop it. I have merged #14. I may think on a simplification of that which won't cause conflict before submitting.

DavisVaughan commented 2 years ago

Just to be extremely clear, now that #14 is merged I think you can submit to CRAN.

This test won't break when we send in the new tidyr, since skip_on_cran() is used.

Then you can follow our release of tidyr by merging this in and removing the Remote. Then you can release an update on your own time

saiemgilani commented 2 years ago

Noted, I'll go ahead and submit then

saiemgilani commented 2 years ago

Submitted to CRAN as of https://github.com/saiemgilani/wehoop/commit/4fce06fd25e911b73a2f142e700bad9cded0c80a