Box universal import linter (#533)

* box universal import linter

* add box_universal_import_linter to .lintr template

* documentation

* package housekeeping

* removed some lint

* namespace box_universal_import_linter

* reordered Suggests

* update e2etest app files to rhino-box styleguide

* address comments/suggestions in PR

* fixed version number issue. moved xml2 to imports

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: rhino
 Title: A Framework for Enterprise Shiny Applications
-Version: 1.6.0.9000
+Version: 1.6.0.9001
 Authors@R:
   c(
     person("Kamil", "Żyła", role = c("aut", "cre"), email = "[email protected]"),
@@ -38,12 +38,14 @@ Imports:
   testthat (>= 3.0.0),
   utils,
   withr,
+  xml2,
   yaml
 Suggests:
   covr,
   knitr,
   mockery,
   rcmdcheck,
+  rex,
   rmarkdown,
   shiny.react,
   spelling

NAMESPACE

@@ -1,6 +1,7 @@
 # Generated by roxygen2: do not edit by hand
 
 export(app)
+export(box_universal_import_linter)
 export(build_js)
 export(build_sass)
 export(diagnostics)

R/box_linters.R

@@ -0,0 +1,57 @@
+#' Box library universal import linter
+#'
+#' Checks that all function imports are explicit. `package[...]` is not used.
+#'
+#' @return A custom linter function for use with `r-lib/lintr`
+#'
+#' @examples
+#' # will produce lints
+#' lintr::lint(
+#'   text = "box::use(base[...])",
+#'   linters = box_universal_import_linter()
+#' )
+#'
+#' lintr::lint(
+#'   text = "box::use(path/to/file[...])",
+#'   linters = box_universal_import_linter()
+#' )
+#'
+#' # okay
+#' lintr::lint(
+#'   text = "box::use(base[print])",
+#'   linters = box_universal_import_linter()
+#' )
+#'
+#' lintr::lint(
+#'   text = "box::use(path/to/file[do_something])",
+#'   linters = box_universal_import_linter()
+#' )
+#'
+#' @export
+box_universal_import_linter <- function() {
+  lint_message <- "Explicitly declare imports rather than universally import with `...`."
+
+  xpath <- "
+  //SYMBOL_PACKAGE[(text() = 'box' and following-sibling::SYMBOL_FUNCTION_CALL[text() = 'use'])]
+    /parent::expr
+    /parent::expr
+    //SYMBOL[text() = '...']
+  "
+
+  lintr::Linter(function(source_expression) {
+    if (!lintr::is_lint_level(source_expression, "file")) {
+      return(list())
+    }
+
+    xml <- source_expression$full_xml_parsed_content
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    lintr::xml_nodes_to_lints(
+      bad_expr,
+      source_expression = source_expression,
+      lint_message = lint_message,
+      type = "style"
+    )
+  })
+}

inst/templates/app_structure/dot.lintr

@@ -1,5 +1,6 @@
 linters:
   linters_with_defaults(
     line_length_linter = line_length_linter(100),
+    box_universal_import_linter = rhino::box_universal_import_linter(),
     object_usage_linter = NULL  # Does not work with `box::use()`.
   )

pkgdown/_pkgdown.yml

@@ -143,6 +143,10 @@ reference:
   - diagnostics
   - test_e2e
 
+- title: Linters
+  contents:
+  - box_universal_import_linter
+
 - title: Data
   contents:
   - rhinos

tests/e2e/app-files/test-hello.R

@@ -1,9 +1,9 @@
 box::use(
   shiny[testServer],
-  testthat[...],
+  testthat[describe, expect_identical, it],
 )
 box::use(
-  app/view/hello[...],
+  app/view/hello[server],
 )
 
 describe("hello$server()", {

tests/e2e/app-files/test-say_hello.R

@@ -1,6 +1,6 @@
-box::use(testthat[...])
+box::use(testthat[describe, expect_identical, it], )
 
-box::use(app/logic/say_hello[say_hello])
+box::use(app/logic/say_hello[say_hello], )
 
 describe("say_hello()", {
   it("should say hello with the correct name", {

tests/testthat/test-linter_box_universal_import_linter.R

@@ -0,0 +1,73 @@
+test_that("box_universal_count_linter skips allowed package import usage", {
+  linter <- box_universal_import_linter()
+
+  good_package_imports <- "box::use(
+      dplyr[select, mutate, ],
+      stringr[str_sub, str_match, ],
+    )
+  "
+
+  lintr::expect_lint(good_package_imports, NULL, linter)
+})
+
+test_that("box_universal_count_linter skips allowed module import usage", {
+  linter <- box_universal_import_linter()
+
+  good_module_imports <- "box::use(
+      path/to/file1[do_something, do_another, ],
+      path/to/file2[find_x, find_y, ],
+    )
+  "
+
+  lintr::expect_lint(good_module_imports, NULL, linter)
+})
+
+test_that("box_universal_count_linter blocks disallowed package import usage", {
+  linter <- box_universal_import_linter()
+
+  bad_package_imports <- "box::use(
+      dplyr[...],
+      stringr[str_sub, str_match, ],
+    )
+  "
+
+  lint_msg <- rex::rex("Explicitly declare imports rather than universally import with `...`.")
+
+  lintr::expect_lint(bad_package_imports, list(message = lint_msg), linter)
+})
+
+test_that("box_universal_count_linter blocks disallowed module import usage", {
+  linter <- box_universal_import_linter()
+
+  bad_module_imports <- "box::use(
+      path/to/file1[...],
+      path/to/file2[find_x, find_y, ],
+    )
+  "
+
+  lint_msg <- rex::rex("Explicitly declare imports rather than universally import with `...`.")
+
+  lintr::expect_lint(bad_module_imports, list(message = lint_msg), linter)
+})
+
+test_that("box_universal_count_linter skips three dots in function declarations and calls", {
+  linter <- box_universal_import_linter()
+
+  function_with_three_dots <- "some_function <- function(...) {
+      sum(...)
+    }
+  "
+
+  lintr::expect_lint(function_with_three_dots, NULL, linter)
+})
+
+test_that("box_universal_count_linter respects #nolint", {
+  linter <- box_universal_import_linter()
+
+  no_lint <- "box::use(
+      shiny[...],         # nolint
+    )
+  "
+
+  lintr::expect_lint(no_lint, NULL, linter)
+})