diff --git a/DESCRIPTION b/DESCRIPTION index a7d5c58a..c7e0496b 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: rhino Title: A Framework for Enterprise Shiny Applications -Version: 1.7.0.9000 +Version: 1.7.0.9001 Authors@R: c( person("Kamil", "Żyła", role = c("aut", "cre"), email = "opensource+kamil@appsilon.com"), diff --git a/NAMESPACE b/NAMESPACE index 251911f8..392b78ac 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,6 +1,7 @@ # Generated by roxygen2: do not edit by hand export(app) +export(box_alphabetical_calls_linter) export(box_func_import_count_linter) export(box_separate_calls_linter) export(box_trailing_commas_linter) diff --git a/NEWS.md b/NEWS.md index 802a08b2..40ece402 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # rhino (development version) +1. New `box_alphabetical_imports_linter` checks if all imports are alphabetically sorted. + # [rhino 1.7.0](https://github.com/Appsilon/rhino/releases/tag/v1.7.0) See _[How-to: Rhino 1.7 Migration Guide](https://appsilon.github.io/rhino/articles/how-to/migrate-1-7.html)_ diff --git a/R/box_linters.R b/R/box_linters.R index f2e70134..987884ff 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -1,3 +1,130 @@ +# nolint start: line_length_linter +#' `box` library alphabetical module and function imports linter +#' +#' Checks that module and function imports are sorted alphabetically. Aliases are +#' ignored. The sort check is on package/module names and attached function names. +#' See the [Explanation: Rhino style guide](https://appsilon.github.io/rhino/articles/explanation/rhino-style-guide.html) +#' to learn about the details. +#' +#' @return A custom linter function for use with `r-lib/lintr`. +#' +#' @examples +#' # will produce lints +#' lintr::lint( +#' text = "box::use(packageB, packageA)", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(package[functionB, functionA])", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/B, path/to/A)", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A[functionB, functionA])", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A[alias = functionB, functionA])", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' # okay +#' lintr::lint( +#' text = "box::use(packageA, packageB)", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(package[functionA, functionB])", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A, path/to/B)", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A[functionA, functionB])", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A[functionA, alias = functionB])", +#' linters = box_alphabetical_calls_linter() +#' ) +#' +#' @export +# nolint end +box_alphabetical_calls_linter <- function() { + xpath_base <- "//SYMBOL_PACKAGE[(text() = 'box' and + following-sibling::SYMBOL_FUNCTION_CALL[text() = 'use'])] + /parent::expr + /parent::expr" + + xpath <- paste(xpath_base, " + /child::expr[ + descendant::SYMBOL + ]") + + xpath_modules_with_functions <- paste(xpath_base, " + /child::expr[ + descendant::SYMBOL and + descendant::OP-LEFT-BRACKET + ]") + + xpath_functions <- "./descendant::expr/SYMBOL[ + ../preceding-sibling::OP-LEFT-BRACKET and + ../following-sibling::OP-RIGHT-BRACKET + ]" + + lint_message <- "Module and function imports must be sorted alphabetically." + + lintr::Linter(function(source_expression) { + if (!lintr::is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + xml_nodes <- xml2::xml_find_all(xml, xpath) + modules_called <- xml2::xml_text(xml_nodes) + modules_check <- modules_called == sort(modules_called) + + unsorted_modules <- which(modules_check == FALSE) + module_lint <- lintr::xml_nodes_to_lints( + xml_nodes[unsorted_modules], + source_expression = source_expression, + lint_message = lint_message, + type = "style" + ) + + xml_nodes_with_functions <- xml2::xml_find_all(xml_nodes, xpath_modules_with_functions) + + function_lint <- lapply(xml_nodes_with_functions, function(xml_node) { + imported_functions <- xml2::xml_find_all(xml_node, xpath_functions) + functions_called <- xml2::xml_text(imported_functions) + functions_check <- functions_called == sort(functions_called) + unsorted_functions <- which(functions_check == FALSE) + + lintr::xml_nodes_to_lints( + imported_functions[unsorted_functions], + source_expression = source_expression, + lint_message = lint_message, + type = "style" + ) + }) + + c(module_lint, function_lint) + }) +} + # nolint start: line_length_linter #' `box` library function import count linter #' diff --git a/inst/templates/app_structure/dot.lintr b/inst/templates/app_structure/dot.lintr index fa3c8d97..c285e634 100644 --- a/inst/templates/app_structure/dot.lintr +++ b/inst/templates/app_structure/dot.lintr @@ -1,6 +1,7 @@ linters: linters_with_defaults( line_length_linter = line_length_linter(100), + box_alphabetical_calls_linter = rhino::box_alphabetical_calls_linter(), box_func_import_count_linter = rhino::box_func_import_count_linter(), box_separate_calls_linter = rhino::box_separate_calls_linter(), box_trailing_commas_linter = rhino::box_trailing_commas_linter(), diff --git a/man/box_alphabetical_calls_linter.Rd b/man/box_alphabetical_calls_linter.Rd new file mode 100644 index 00000000..6010a604 --- /dev/null +++ b/man/box_alphabetical_calls_linter.Rd @@ -0,0 +1,71 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/box_linters.R +\name{box_alphabetical_calls_linter} +\alias{box_alphabetical_calls_linter} +\title{\code{box} library alphabetical module and function imports linter} +\usage{ +box_alphabetical_calls_linter() +} +\value{ +A custom linter function for use with \code{r-lib/lintr}. +} +\description{ +Checks that module and function imports are sorted alphabetically. Aliases are +ignored. The sort check is on package/module names and attached function names. +See the \href{https://appsilon.github.io/rhino/articles/explanation/rhino-style-guide.html}{Explanation: Rhino style guide} +to learn about the details. +} +\examples{ +# will produce lints +lintr::lint( + text = "box::use(packageB, packageA)", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(package[functionB, functionA])", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(path/to/B, path/to/A)", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(path/to/A[functionB, functionA])", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(path/to/A[alias = functionB, functionA])", + linters = box_alphabetical_calls_linter() +) + +# okay +lintr::lint( + text = "box::use(packageA, packageB)", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(package[functionA, functionB])", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(path/to/A, path/to/B)", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(path/to/A[functionA, functionB])", + linters = box_alphabetical_calls_linter() +) + +lintr::lint( + text = "box::use(path/to/A[functionA, alias = functionB])", + linters = box_alphabetical_calls_linter() +) + +} diff --git a/pkgdown/_pkgdown.yml b/pkgdown/_pkgdown.yml index ce926b57..c9bc81df 100644 --- a/pkgdown/_pkgdown.yml +++ b/pkgdown/_pkgdown.yml @@ -149,6 +149,7 @@ reference: - title: Linters contents: + - box_alphabetical_calls_linter - box_func_import_count_linter - box_separate_calls_linter - box_trailing_commas_linter diff --git a/tests/testthat/test-linter_box_alphabetical.R b/tests/testthat/test-linter_box_alphabetical.R new file mode 100644 index 00000000..e6ae9924 --- /dev/null +++ b/tests/testthat/test-linter_box_alphabetical.R @@ -0,0 +1,136 @@ +test_that("box_alphabetical_calls_linter() skips allowed box::use() calls", { + linter <- box_alphabetical_calls_linter() + + good_box_calls_1 <- "box::use( + dplyr, + shiny, + tidyr, + )" + + good_box_calls_2 <- "box::use( + path/to/fileA, + path/to/fileB, + path/to/fileC, + )" + + good_box_calls_3 <- "box::use( + dplyr[filter, mutate, select], + shiny, + tidyr[long, pivot, wide], + )" + + good_box_calls_4 <- "box::use( + path/to/fileA[functionA, functionB, functionC], + path/to/fileB, + path/to/fileC[functionD, functionE, functionF], + )" + + lintr::expect_lint(good_box_calls_1, NULL, linter) + lintr::expect_lint(good_box_calls_2, NULL, linter) + lintr::expect_lint(good_box_calls_3, NULL, linter) + lintr::expect_lint(good_box_calls_4, NULL, linter) +}) + +test_that("box_alphabetical_calls_linter() ignores aliases in box::use() calls", { + linter <- box_alphabetical_calls_linter() + + good_box_calls_alias_1 <- "box::use( + dplyr, + alias = shiny, + tidyr, + )" + + good_box_calls_alias_2 <- "box::use( + path/to/fileA, + a = path/to/fileB, + path/to/fileC, + )" + + good_box_calls_alias_3 <- "box::use( + dplyr[filter, alias = mutate, select], + shiny, + tidyr[long, pivot, wide], + )" + + good_box_calls_alias_4 <- "box::use( + path/to/fileA[functionA, alias = functionB, functionC], + path/to/fileB, + path/to/fileC[functionD, functionE, functionF], + )" + + lintr::expect_lint(good_box_calls_alias_1, NULL, linter) + lintr::expect_lint(good_box_calls_alias_2, NULL, linter) + lintr::expect_lint(good_box_calls_alias_3, NULL, linter) + lintr::expect_lint(good_box_calls_alias_4, NULL, linter) +}) + +test_that("box_alphabetical_calls_linter() blocks unsorted imports in box::use() call", { + linter <- box_alphabetical_calls_linter() + + bad_box_calls_1 <- "box::use( + dplyr, + tidyr, + shiny, + )" + + bad_box_calls_2 <- "box::use( + path/to/fileC, + path/to/fileB, + path/to/fileA, + )" + + bad_box_calls_3 <- "box::use( + dplyr[filter, mutate, select], + shiny, + tidyr[wide, pivot, long], + )" + + bad_box_calls_4 <- "box::use( + dplyr[select, mutate, filter], + shiny, + tidyr[wide, pivot, long], + )" + + bad_box_calls_5 <- "box::use( + path/to/fileA[functionC, functionB, functionA], + path/to/fileB, + path/to/fileC[functionD, functionE, functionF], + )" + + bad_box_calls_6 <- "box::use( + path/to/fileA[functionC, functionB, functionA], + path/to/fileB, + path/to/fileC[functionF, functionE, functionD], + )" + + lint_message <- rex::rex("Module and function imports must be sorted alphabetically.") + + lintr::expect_lint(bad_box_calls_1, list( + list(message = lint_message, line_number = 3), + list(message = lint_message, line_number = 4) + ), linter) + lintr::expect_lint(bad_box_calls_2, list( + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 4) + ), linter) + lintr::expect_lint(bad_box_calls_3, list( + list(message = lint_message, line_number = 4, column_number = 11), + list(message = lint_message, line_number = 4, column_number = 24) + ), linter) + lintr::expect_lint(bad_box_calls_4, list( + list(message = lint_message, line_number = 2, column_number = 11), + list(message = lint_message, line_number = 2, column_number = 27), + list(message = lint_message, line_number = 4, column_number = 11), + list(message = lint_message, line_number = 4, column_number = 24) + ), linter) + lintr::expect_lint(bad_box_calls_5, list( + list(message = lint_message, line_number = 2, column_number = 19), + list(message = lint_message, line_number = 2, column_number = 41) + ), linter) + lintr::expect_lint(bad_box_calls_6, list( + list(message = lint_message, line_number = 2, column_number = 19), + list(message = lint_message, line_number = 2, column_number = 41), + list(message = lint_message, line_number = 4, column_number = 19), + list(message = lint_message, line_number = 4, column_number = 41) + ), linter) +}) diff --git a/vignettes/explanation/box-modules.Rmd b/vignettes/explanation/box-modules.Rmd index 2629136e..41ec9fff 100644 --- a/vignettes/explanation/box-modules.Rmd +++ b/vignettes/explanation/box-modules.Rmd @@ -61,7 +61,7 @@ Both `say_hello()` and `say_bye()` can be exported from `app/logic/messages.R`. ```r box::use( - app/logic/messages[say_hello, say_bye], + app/logic/messages[say_bye, say_hello], ) #' @export @@ -79,7 +79,7 @@ Modules can also be imported across directories; use code from `app/logic` in `a ```r # app/view/greet_module.R box::use( - shiny[moduleServer, NS, renderText, div, textOutput, req], + shiny[div, moduleServer, NS, renderText, req, textOutput], shiny.semantic[textInput], ) diff --git a/vignettes/explanation/rhino-style-guide.Rmd b/vignettes/explanation/rhino-style-guide.Rmd index a6914789..393410bc 100644 --- a/vignettes/explanation/rhino-style-guide.Rmd +++ b/vignettes/explanation/rhino-style-guide.Rmd @@ -90,6 +90,24 @@ box::use( ) ``` +## Aliases + +Aliases can be useful for long package/module and function names. Imports should still follow the alphabetical order of package/module names and function names. + +```r +# Good +box::use( + z_pkg = rhino, + shiny[div, a_fun = fluidPage], +) + +# Bad +box::use( + a_pkg = shiny, + rhino[a_fun = react_component, log], +) +``` + # Number of Imports Limit the number of functions imported from a module or package to 8.