jthomasmock / gtExtras

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

Housekeeping Part 2: (╯°□°)╯︵ ┻━┻ #44

Closed ddsjoberg closed 2 years ago

ddsjoberg commented 2 years ago
  1. Removed all NOTES about undefined global variables in the R CMD Check. This required adding many many many .data references.
  2. Removed tidyverse dependency, so you will be able to go to CRAN when you're ready. Moved webshot2 dependency from Imports to Suggests.
  3. Removed references to webshot pkg (and dependency from DESCRIPTION)
  4. Added a package level documentation file with usethis::use_package_doc() and consolidated all roxygen2 @import calls to this documentation file.
  5. Registered global variable "." in the package documentation file.
  6. On my machine, I was getting an error in R/fontawesome-icons.R stemming from line if(is.null(palette) && unique(int_x) >= 8). I updated it to if(is.null(palette) && length(unique(int_x)) >= 8). Please confirm this is accurate.
  7. Added documentation of parameters as needed, and deleted unused parameter documentation.
  8. Reconciled different names in the title and vignette name in plots-in-gt.Rmd

Outstanding Items:

  1. The updates above take care of all NOTES from R CMD Checks except those arising from the use of internal gt functions accessed with gt:::. Before you submit to CRAN, you'll need to resolve this note. There are ways to conceal calls to other package's internal functions from CRAN. But if you get caught, the package will be rejected.
  2. I see that you made some updates while I was also making updates. I'll need to resolve some merge conflicts before you review :)
ddsjoberg commented 2 years ago

@jthomasmock The merge conflicts were easier to resolve than I thought they would be! This PR is ready for review! ٩(^ ᴗ ^)۶

jthomasmock commented 2 years ago

You are an absolute saint, hope you know that! 😭

Reviewing, also wanted to confirm - are you ok being indicated as a contributor?

I would definitely say that you have left your mark on the package to say the least.

jthomasmock commented 2 years ago

On my machine, I was getting an error in R/fontawesome-icons.R stemming from line if(is.null(palette) && unique(int_x) >= 8). I updated it to if(is.null(palette) && length(unique(int_x)) >= 8). Please confirm this is accurate.

Interesting that this wasn't erroring locally for me. The length(unique(int_x)) >= 8 is the intended behavior. Thanks!

ddsjoberg commented 2 years ago

You're a saint for writing such a beautiful pkg!! Happy to be listed as a contributor: that is so kind :)

One other thing that'll need to be taken care of before CRAN: webshot2. CRAN will allow you a weak dependency (aka a package in Suggests) that is not on CRAN, but it must be housed in a proper pkg repository (i.e. they do not allow for Remotes: rstudio/webshot2 in the DESCRIPTION).

There are two pretty straightforward options:

  1. Add an R Universe repo for your jthomasmock GH account. You can then add webshot2 to your universe. You'll then add this line to the DESCRIPTION Additional_repositories: https://jthomasmock .r-universe.dev Here's what mine looks like https://ddsjoberg.r-universe.dev/ I use gtExtras in my bstfun pkg, so gtExtras is auto-added to my universe.
  2. You can create a drat repo and add it there. Then add a corresponding Additional_repositories: line to DESCRIPTION.
jthomasmock commented 2 years ago

Thanks again, I've been meaning to setup a r-universe, so this is a great excuse 😄

There are ways to conceal calls to other package's internal functions from CRAN. But if you get caught, the package will be rejected.

One more question re: gt::: - I would prefer to stick with those as it's unfortunately a lot of vendored code otherwise. I'm taking a shot at removing their use, but if you have any suggestions I'd love to hear it!

ddsjoberg commented 2 years ago

Ooo, this is almost ready for CRAN!

I feel funny about putting this advice in a public space 😆 😆 😆

  1. if these are functions that are helpful for you, there is a good chance they are helpful for others. Perhaps ask Rich if he can export them? This is the easiest solution and it's above-board.
  2. You can wrap the calls in do.call() and quote the function name, e.g. do.call("foo", list(arg1, arg2)) Since the function name is in quotes, it won't be picked up by the R CMD Check. This has some risk, however. If Rich were to change any of his internal functions, then gtExtras will break.
  3. An alternative would be to save a copy of these functions internally to the package as an object with usethis::use_data(). This will save a copy inside your package. If the function you save has other internal functions that are called from gt, I am not sure if they'll be found however. Would need to do some testing.
  4. I've seen some other solution that imports the function through some sort of NAMESPACE call. But I don't recall the details.
jthomasmock commented 2 years ago

Good to know - I was able to get it all vendored, but may revisit as it's quite a bit of code!!!

I've bumped to v3.0 in prep for hopefully a CRAN release soon. Added you as a ctr, thanks so much!