Box universal import linter (#555)

* future dev branch

* update rhino app template unit test (#539)

* update rhino app template unit test

* update e2etest app files

* remove what looks like stylr styling

* one version too far

* 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

* chore: Update NEWS and add Rodrigo to authors.

---------

Co-authored-by: Jakub Nowicki <[email protected]>

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]"),
@@ -9,6 +9,7 @@ Authors@R:
     person("Marek", "Rogala", role = "aut", email = "[email protected]"),
     person("Recle", "Vibal", role = "aut", email = "[email protected]"),
     person("Tymoteusz", "Makowski", role = "aut", email = "[email protected]"),
+    person("Rodrigo", "Basa", role = "aut", email = "[email protected]"),
     person("Eduardo", "Almeida", role = "ctb", email = "[email protected]"),
     person("Appsilon Sp. z o.o.", role = "cph", email = "[email protected]")
   )
@@ -38,12 +39,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)

NEWS.md

@@ -1,5 +1,8 @@
 # rhino (development version)
 
+1. Introduce linters for `box::use` statements:
+    * `box_universal_import_linter` checks if all imports are explicit.
+
 # [rhino 1.6.0](https://github.com/Appsilon/rhino/releases/tag/v1.6.0)
 
 1. `pkg_install` supports installation from local sources, GitHub, and Bioconductor.

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()`.
   )

inst/templates/unit_tests/tests/testthat/test-main.R

@@ -1,9 +1,9 @@
 box::use(
   shiny[testServer],
-  testthat[...],
+  testthat[expect_true, test_that],
 )
 box::use(
-  app/main[...],
+  app/main[server, ui],
 )
 
 test_that("main server works", {

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/hello.R

@@ -1,20 +1,20 @@
 box::use(
   shiny[
-    bootstrapPage,
-    NS,
-    tags,
-    textInput,
     actionButton,
-    observeEvent,
-    textOutput,
+    bootstrapPage,
+    isolate,
     moduleServer,
+    NS,
     observe,
+    observeEvent,
     renderText,
-    isolate
-  ]
+    tags,
+    textInput,
+    textOutput
+  ],
 )
 
-box::use(app / logic / say_hello[say_hello])
+box::use(app/logic/say_hello[say_hello])
 
 #' @export
 ui <- function(id) {

tests/e2e/app-files/main.R

@@ -1,9 +1,9 @@
 box::use(
+  rhino[log, react_component],
   shiny,
-  rhino[log, react_component]
 )
 
-box::use(app / view / hello)
+box::use(app/view/hello)
 
 Box <- react_component("Box") # nolint object_name_linter
 

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)
+})