Resolve R CMD check note about 'scales' package

[arni-magnusson]

This pull request resolves a Note raised by R CMD check, moving the scales package from Imports to Suggests.

The scales package is not imported from, so I'm guessing it's not required as an Imports entry. Not a big issue - just trying to keep R CMD check happy :)

[kellijohnson-NOAA]

Thanks @arni-magnusson for the PR. I am not entirely sure why it thinks that we do not need scales because it is used in some {ggplot2} code here

FIMS/R/fimsframe.R

Line 183 in f0d4e76

ggplot2::scale_x_date(labels = scales::date_format("%Y-%m-%d")) +

Though we should change to scales::label_date() per the documentation for scales::date_format() but that is a different issue.

I always thought that if a package is used within the code it should be in imports and if it is used by developers say for creating data sets like {usethis} can be in the data-raw folder, then it should be in Suggests. Feel free to correct me if I am wrong.

[arni-magnusson]

You're absolutely right. At closer inspection, I can see that the scales package should indeed be in the Imports field of the DESCRIPTION file, not in Suggests.

This Note from R CMD check is effectively a false positive. I hadn't come across this case of false positive from R CMD check before, as I tend to use @importFrom pkg function for functions that I use. I fully appreciate if the FIMS style is not to import every function that is used, following the recommendation in Wickham and Bryan's book on R Packages, section 11.4.1.

Wickham and Bryan point to a workaround solution in their section 11.4.1.1, defining a dummy ignore_unused_imports() function. I've modified the pull request to apply this solution, also used in devtools. Feel free to ignore the pull request if this solution seems clunky :)

An alternative solution pointed out by Wickham and Bryan section 11.4.1.1 would be to add this entry to FIMS-package.R:
#' @importFrom scales date_format

[kellijohnson-NOAA]

@arni-magnusson thank you for your pull request. Upon further investigation, I believe that this is happening because {scales} is used inside of methods::setMethod() and not as standalone code in a normal function. I am going to figure out exactly what call the note is being generated from in devtools::check() so I can report it as a formal issue and then hopefully we will not have to have a workaround. I may implement your current suggestion in dev if we do not get a more permanent solution by the next release. So, I will leave this PR open as a reminder.

Second, all future contributions should be made to dev and not main. This is a new protocol for us as of ~ June so it is not well documented. I will try to fix that.

[kellijohnson-NOAA]

I finally figured it out 😓! Line 169 in the following code is incorrect

FIMS/R/fimsframe.R

Lines 166 to 177 in f0d4e76

	setMethod(
	f = "plot",
	signature = "FIMSFrame",
	definition = function(x) {
	ggplot2::ggplot(
	data = x@data,
	mapping = ggplot2::aes(
	x = as.Date(.data[["datestart"]]),
	y = .data[["value"]],
	col = .data[["name"]]
	)
	) +

It should be

setMethod(
  f = "plot",
  signature = "FIMSFrame",
  definition = function(x, y, ...) {
    ggplot2::ggplot(
      data = x@data,
      mapping = ggplot2::aes(
        x = as.Date(.data[["datestart"]]),
        y = .data[["value"]],
        col = .data[["name"]]
      )
    ) +

because the args(plot) are plot(x, y, ...) and the addition to the generic method needs to have the same arguments or more if ... is present. My original code only has x, which was causing lapply(exprs, deparse), where exprs are all code chunks in the package, to call out the plot method as a .local function rather than a standard function. This later leads to find_bad_exprs() not being able to pick up a package being used in the code chunk. FYI all of this is in R CMD check --as-cran or in tools:::.check_packages_used().

@arni-magnusson let me know if you would like to modify your PR to have proposed fix to dev or I can open a new PR. Thanks again 🙏 for bringing this up. Although, it turned into quite the 🐰 🕳️ it was a fun one.

Below are some references that helped me find some answers.

• tools:::.check_packages_used()
• "All declared Imports should be used"
• stack overflow on how to declare an s3 or s4 dependency
• GitHub Issue on r-pkgs mentioned in previous stack overflow post
• R bug report
• Another R bug report, when package name is in quotes

[arni-magnusson]

Thanks for the investigation and following up, @kellijohnson-NOAA. I've modified the PR to target the dev branch. That was a new button for me to learn about in GitHub :)

[kellijohnson-NOAA]

@arni-magnusson thank you for this PR I have fixed it in a20b806 with a slightly different method than what you outlined and I described it above. The warning is now taken care of. Thank you again for the contributions and bringing it to my attention.