Appsilon / box.linters

lintr-compatible linters for box modules in R
https://appsilon.github.io/box.linters/
10 stars 0 forks source link

[LINT_BUG]: Multiple box::use() calls cause lint to fail #111

Closed caldwellst closed 4 months ago

caldwellst commented 5 months ago

box.linters version

0.9.1

Sample source code to lint

box::use(dplyr)
box::use(stringr)

#' @export
x <- function() {
  dplyr$mutate(mtcars)
  stringr$str_detect("a", "a")
}

Lint command used

lintr::lint(
  code,
  linters = lintr::linters_with_defaults(defaults = box.linters::box_default_linters)
)

Lint result

Line 1 [box_unused_att_pkg_linter] Attached package unused.
Line 2 [box_unused_att_pkg_linter] Attached package unused.
Line 6 [box_usage_linter] <package/module>$function does not exist.
Line 7 [box_usage_linter] <package/module>$function does not exist.

Expected result

This should succeed. Linting is successful if we only import a single package.

box::use(dplyr)

#' @export
x <- function() {
  dplyr$mutate(mtcars)
}

It is also successful with multiple imports if we follow the box.linters::rhino_default_linters style of adding , after all imports.

box::use(dplyr, )
box::use(stringr, )

#' @export
x <- function() {
  dplyr$mutate(mtcars)
  stringr$str_detect("a", "a")
}

And if we only add , without the space, it is also successful, which leads me to suspect it has something to do with expecting comma separation that allows the linter to successfully find multiple imports.

radbasa commented 5 months ago

This is interesting.

caldwellst commented 5 months ago

Just adding on here that I get similar, but slightly different issues, when testing against modules. Here, just have a.R importing from src/z.R and src/y.R.

a.R

box::use(src/x)
box::use(src/y)

x$x()
y$y()

And running:

lintr::lint("x.R", linters = box.linters::box_default_linters)

Gives:

Line 1 [box_unused_attached_mod_linter] Attached module unused.
Line 4 [box_usage_linter] <package/module>$function does not exist.

Interesting because one of the attachments succeeds.

a.R

box::use(src/x)
box::use(src/y)
box::use(src/z)

x$x()
y$y()
z$z()

And this generates errors for the first two attachments src/x and src/y, but again, the final attachment passes linting, in this case src/z.

Using the box::use(src/x, ) style ensures that all linting passes, just like when attaching packages.

radbasa commented 5 months ago

While linting box::use(packages) use a different set of functions from linting box::use(path/modules), they both follow the same algorithm.

We, at Appsilon, as standard practice use one box::use() call for packages, and a separate box::use() call for modules. We did not take into consideration having multiple box::use() calls. We will include this feature request in future releases.

Thanks.

radbasa commented 4 months ago

@caldwellst We just released 0.10.0 on CRAN. It comes with a fix for this issue.