diff --git a/DESCRIPTION b/DESCRIPTION index 202cdcf7..119a62de 100644 --- a/DESCRIPTION +++ b/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 = "opensource+kamil@appsilon.com"), @@ -9,6 +9,7 @@ Authors@R: person("Marek", "Rogala", role = "aut", email = "marek@appsilon.com"), person("Recle", "Vibal", role = "aut", email = "recle.vibal@appsilon.com"), person("Tymoteusz", "Makowski", role = "aut", email = "tymoteusz@appsilon.com"), + person("Rodrigo", "Basa", role = "aut", email = "rodrigo@appsilon.com"), person("Eduardo", "Almeida", role = "ctb", email = "eduardo@appsilon.com"), person("Appsilon Sp. z o.o.", role = "cph", email = "opensource@appsilon.com") ) @@ -38,12 +39,14 @@ Imports: testthat (>= 3.0.0), utils, withr, + xml2, yaml Suggests: covr, knitr, mockery, rcmdcheck, + rex, rmarkdown, shiny.react, spelling diff --git a/NAMESPACE b/NAMESPACE index 2b25b2d5..521311fc 100644 --- a/NAMESPACE +++ b/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) diff --git a/NEWS.md b/NEWS.md index b1d65880..80a526bb 100644 --- a/NEWS.md +++ b/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. diff --git a/R/box_linters.R b/R/box_linters.R new file mode 100644 index 00000000..f0ba769c --- /dev/null +++ b/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" + ) + }) +} diff --git a/inst/templates/app_structure/dot.lintr b/inst/templates/app_structure/dot.lintr index 63fade9a..7dff4f80 100644 --- a/inst/templates/app_structure/dot.lintr +++ b/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()`. ) diff --git a/inst/templates/unit_tests/tests/testthat/test-main.R b/inst/templates/unit_tests/tests/testthat/test-main.R index f9d795b7..003ae7e8 100644 --- a/inst/templates/unit_tests/tests/testthat/test-main.R +++ b/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", { diff --git a/man/box_universal_import_linter.Rd b/man/box_universal_import_linter.Rd new file mode 100644 index 00000000..f4a8057a --- /dev/null +++ b/man/box_universal_import_linter.Rd @@ -0,0 +1,38 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/box_linters.R +\name{box_universal_import_linter} +\alias{box_universal_import_linter} +\title{Box library universal import linter} +\usage{ +box_universal_import_linter() +} +\value{ +A custom linter function for use with \code{r-lib/lintr} +} +\description{ +Checks that all function imports are explicit. \code{package[...]} is not used. +} +\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() +) + +} diff --git a/pkgdown/_pkgdown.yml b/pkgdown/_pkgdown.yml index 759c93d4..6b5dc810 100644 --- a/pkgdown/_pkgdown.yml +++ b/pkgdown/_pkgdown.yml @@ -143,6 +143,10 @@ reference: - diagnostics - test_e2e +- title: Linters + contents: + - box_universal_import_linter + - title: Data contents: - rhinos diff --git a/tests/e2e/app-files/hello.R b/tests/e2e/app-files/hello.R index 58bfbef1..19023488 100644 --- a/tests/e2e/app-files/hello.R +++ b/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) { diff --git a/tests/e2e/app-files/main.R b/tests/e2e/app-files/main.R index c789ebc0..f18ba6ab 100644 --- a/tests/e2e/app-files/main.R +++ b/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 diff --git a/tests/e2e/app-files/test-hello.R b/tests/e2e/app-files/test-hello.R index 54301473..a8d0a96a 100644 --- a/tests/e2e/app-files/test-hello.R +++ b/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()", { diff --git a/tests/e2e/app-files/test-say_hello.R b/tests/e2e/app-files/test-say_hello.R index 84b393c6..9ad2d999 100644 --- a/tests/e2e/app-files/test-say_hello.R +++ b/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", { diff --git a/tests/testthat/test-linter_box_universal_import_linter.R b/tests/testthat/test-linter_box_universal_import_linter.R new file mode 100644 index 00000000..1f4e39bb --- /dev/null +++ b/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) +})