From fea7b836cad52f30756cc35d33b82782fbe35a2f Mon Sep 17 00:00:00 2001 From: Ricardo Rodrigo Basa Date: Fri, 26 Jan 2024 12:26:00 +0800 Subject: [PATCH] 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 | 4 +- NAMESPACE | 1 + R/box_linters.R | 57 +++++++++++++++ inst/templates/app_structure/dot.lintr | 1 + man/box_universal_import_linter.Rd | 38 ++++++++++ pkgdown/_pkgdown.yml | 4 + tests/e2e/app-files/test-hello.R | 4 +- tests/e2e/app-files/test-say_hello.R | 4 +- .../test-linter_box_universal_import_linter.R | 73 +++++++++++++++++++ 9 files changed, 181 insertions(+), 5 deletions(-) create mode 100644 R/box_linters.R create mode 100644 man/box_universal_import_linter.Rd create mode 100644 tests/testthat/test-linter_box_universal_import_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 202cdcf7..94e59bb0 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"), @@ -38,12 +38,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/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/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/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) +})