jthomasmock / gtExtras

A Collection of Helper Functions for the gt Package.
https://jthomasmock.github.io/gtExtras/
Other
195 stars 27 forks source link

Minor housekeeping updates #43

Closed ddsjoberg closed 2 years ago

ddsjoberg commented 2 years ago

Hello hello @jthomasmock !

I saw that there were some recent updates adding webshot2 (which is great!). It looks like there were two small issues with this change. 1. webshot2 isn't on CRAN, so we need to let the DESCRIPTION file know where to download the pkg from, and 2. due to this line @importFrom webshot2 webshot webshot2 must go in Imports: rather than Suggests because it's required to build the package. I use gtExtras in one of my packages, and the recent change is causing R CMD Check failures on GH Actions because the gtExtras cannot be built from source.

The following minor housekeeping updates were made:

  1. Moved webshot2 to Imports from Suggests
  2. Added Remotes: rstudio/webshot2 to the DESCRIPTION file so R will know where to install webshot2 from.
  3. Added the docs folder and the pkgdown yaml to .Rbuildignore to remove a NOTE during R CMD Check.
  4. Added packages nflreadr and tidyverse to the Suggests section of the DESCRIPTION file as these are used in the pkg documentation. When you go to CRAN, you will need to remove the tidyverse dependency and swap it out for the individual packages you use.
  5. I exported the pipe operator so you don't need to load magrittr (or another pkg) to use it with gtExtras. This fixes as error in one of the examples where the pipe had not been loaded, but was being used.
  6. I added the stats:: prefix to functions from that package to remove notes in the R CMD Check, e.g. median() is now stats::median()

Thank you for the vvvvv cute package!!!

jthomasmock commented 2 years ago

Howdy @ddsjoberg ! Thanks for the note and PR!

I have some code in the gtsave_extra() function to suggest installation of webshot2 if it's not installed. I'd prefer it to be a suggests than a hard dependency especially as it's not on CRAN.

If I could remove the @importfrom webshot2 would it be ok to move it back to suggests?


Everything else looks good to me - and I'll likely accept the PR as-is but wanted to confirm on the depends/suggests.

ddsjoberg commented 2 years ago

Hey hey @jthomasmock !

Yep, I think if you remove @importfrom webshot2 webshot you can safely move webshot2 back to Suggests (because you do have that code to check the installation already).

Just as an FYI, there are references to webshot that still appear in the documentation and in one place in the codebase. Once those are updated to webshot2, you can remove the webshot dependency altogether.

ddsjoberg commented 2 years ago

Thanks @jthomasmock ! If you make updates to get the unit tests to pass, I can continue with other housekeeping items to clean up the R CMD Checks (e.g. removing notes and warnings, move webshot2 to Suggests, remove webshot?, etc.)

jthomasmock commented 2 years ago

Thank you so much for the help! 🙏

Once I can get the R Cmd Checks satisfied, I'll be much closer to a first CRAN release.

I don't have failing tests currently.

══ Results ═════════════
Duration: 32.1 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 169 ]

🎉🎊🎉🎊🎉🎊🎉🎊🎉🎊🎉

but man alive do I have a lot of no visible binding, since I'm relying on lapply() for a lot of internal code with anonymous functions.