From e8e7bbf62f3232c8dfd0891cbca41f064f3e7ac8 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 12 Jan 2024 19:11:23 +0800 Subject: [PATCH 01/16] initial attempt. unfinished. can check packages/modules. cannot check functions. --- DESCRIPTION | 2 +- R/linter_box_alphabetical.R | 39 +++++++++++++++++++ tests/testthat/test-linter_box_alphabetical.R | 3 ++ 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 R/linter_box_alphabetical.R create mode 100644 tests/testthat/test-linter_box_alphabetical.R diff --git a/DESCRIPTION b/DESCRIPTION index f399da4f..ba7a089a 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.9005 Authors@R: c( person("Kamil", "Żyła", role = c("aut", "cre"), email = "opensource+kamil@appsilon.com"), diff --git a/R/linter_box_alphabetical.R b/R/linter_box_alphabetical.R new file mode 100644 index 00000000..0146a363 --- /dev/null +++ b/R/linter_box_alphabetical.R @@ -0,0 +1,39 @@ +box_alphabetical_imports <- 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 <- "child::expr[1]/SYMBOL | child::SYMBOL" + + lint_message <- "Must be alphabetical." + + lintr::Linter(function(source_expression) { + if (!lintr::is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + xml_root <- xml2::xml_find_all(xml, xpath_base) + + xml_nodes <- xml2::xml_find_all(xml, xpath) + + modules_called <- xml2::xml_text( + xml2::xml_find_all(xml_nodes, xpath_modules) + ) + + if (!all(modules_called == sort(modules_called))) { + lintr::xml_nodes_to_lints( + xml_root, + source_expression = source_expression, + lint_message = lint_message, + type = "style" + ) + } + }) +} diff --git a/tests/testthat/test-linter_box_alphabetical.R b/tests/testthat/test-linter_box_alphabetical.R new file mode 100644 index 00000000..8849056e --- /dev/null +++ b/tests/testthat/test-linter_box_alphabetical.R @@ -0,0 +1,3 @@ +test_that("multiplication works", { + expect_equal(2 * 2, 4) +}) From 77c000f584968a98b1d0affb385e375a25c52a94 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 16:48:33 +0800 Subject: [PATCH 02/16] add alphabetical imports linter --- R/box_linters.R | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/R/box_linters.R b/R/box_linters.R index f2e70134..d41dd0fb 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -1,3 +1,126 @@ +# nolint start: line_length_linter +#' `box` library alphabetical module and function imports linter +#' +#' Checks that module and function imports are sorted alphabetically. +#' 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_imports_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(package[functionB, functionA])", +#' linters = box_func_import_count_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/B, path/to/A)", +#' linters = box_alphabetical_imports_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A[functionB, functionA])", +#' linters = box_alphabetical_imports_linter() +#' ) +#' +#' # okay +#' lintr::lint( +#' text = "box::use(packageA, packageB)", +#' linters = box_func_import_count_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(package[one, two, three])", +#' linters = box_func_import_count_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(package[functionA, functionB])", +#' linters = box_func_import_count_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A, path/to/B)", +#' linters = box_alphabetical_imports_linter() +#' ) +#' +#' lintr::lint( +#' text = "box::use(path/to/A[functionA, functionB])", +#' linters = box_alphabetical_imports_linter() +#' ) +#' +#' @export +# nolint end +box_alphabetical_imports_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 <- "./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_root <- xml2::xml_find_all(xml, xpath_base) + 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 #' From 787c823484083902b5938ec1f5008fd37f87503e Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 16:49:38 +0800 Subject: [PATCH 03/16] alphabetical box linter support code --- NAMESPACE | 1 + R/linter_box_alphabetical.R | 39 ------ man/box_alphabetical_imports_linter.Rd | 65 +++++++++ tests/testthat/test-linter_box_alphabetical.R | 128 +++++++++++++++++- 4 files changed, 192 insertions(+), 41 deletions(-) delete mode 100644 R/linter_box_alphabetical.R create mode 100644 man/box_alphabetical_imports_linter.Rd diff --git a/NAMESPACE b/NAMESPACE index 251911f8..7f08cdd7 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,6 +1,7 @@ # Generated by roxygen2: do not edit by hand export(app) +export(box_alphabetical_imports_linter) export(box_func_import_count_linter) export(box_separate_calls_linter) export(box_trailing_commas_linter) diff --git a/R/linter_box_alphabetical.R b/R/linter_box_alphabetical.R deleted file mode 100644 index 0146a363..00000000 --- a/R/linter_box_alphabetical.R +++ /dev/null @@ -1,39 +0,0 @@ -box_alphabetical_imports <- 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 <- "child::expr[1]/SYMBOL | child::SYMBOL" - - lint_message <- "Must be alphabetical." - - lintr::Linter(function(source_expression) { - if (!lintr::is_lint_level(source_expression, "expression")) { - return(list()) - } - - xml <- source_expression$xml_parsed_content - - xml_root <- xml2::xml_find_all(xml, xpath_base) - - xml_nodes <- xml2::xml_find_all(xml, xpath) - - modules_called <- xml2::xml_text( - xml2::xml_find_all(xml_nodes, xpath_modules) - ) - - if (!all(modules_called == sort(modules_called))) { - lintr::xml_nodes_to_lints( - xml_root, - source_expression = source_expression, - lint_message = lint_message, - type = "style" - ) - } - }) -} diff --git a/man/box_alphabetical_imports_linter.Rd b/man/box_alphabetical_imports_linter.Rd new file mode 100644 index 00000000..384548d0 --- /dev/null +++ b/man/box_alphabetical_imports_linter.Rd @@ -0,0 +1,65 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/box_linters.R +\name{box_alphabetical_imports_linter} +\alias{box_alphabetical_imports_linter} +\title{\code{box} library alphabetical module and function imports linter} +\usage{ +box_alphabetical_imports_linter() +} +\value{ +A custom linter function for use with \code{r-lib/lintr}. +} +\description{ +Checks that module and function imports are sorted alphabetically. +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_imports_linter() +) + +lintr::lint( + text = "box::use(package[functionB, functionA])", + linters = box_func_import_count_linter() +) + +lintr::lint( + text = "box::use(path/to/B, path/to/A)", + linters = box_alphabetical_imports_linter() +) + +lintr::lint( + text = "box::use(path/to/A[functionB, functionA])", + linters = box_alphabetical_imports_linter() +) + +# okay +lintr::lint( + text = "box::use(packageA, packageB)", + linters = box_func_import_count_linter() +) + +lintr::lint( + text = "box::use(package[one, two, three])", + linters = box_func_import_count_linter() +) + +lintr::lint( + text = "box::use(package[functionA, functionB])", + linters = box_func_import_count_linter() +) + +lintr::lint( + text = "box::use(path/to/A, path/to/B)", + linters = box_alphabetical_imports_linter() +) + +lintr::lint( + text = "box::use(path/to/A[functionA, functionB])", + linters = box_alphabetical_imports_linter() +) + +} diff --git a/tests/testthat/test-linter_box_alphabetical.R b/tests/testthat/test-linter_box_alphabetical.R index 8849056e..04502387 100644 --- a/tests/testthat/test-linter_box_alphabetical.R +++ b/tests/testthat/test-linter_box_alphabetical.R @@ -1,3 +1,127 @@ -test_that("multiplication works", { - expect_equal(2 * 2, 4) +test_that("box_alphabetical_imports_linter skips allowed box::use() calls", { + linter <- box_alphabetical_imports_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_imports_linter ignores aliases in box::use() calls", { + linter <- box_alphabetical_imports_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_imports_linter blocks unsorted imports in box::use() call", { + linter <- box_alphabetical_imports_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), + list(message = lint_message, line_number = 4) + ), linter) + lintr::expect_lint(bad_box_calls_4, list( + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) + ), linter) + lintr::expect_lint(bad_box_calls_5, list(message = lint_message), linter) + lintr::expect_lint(bad_box_calls_6, list(message = lint_message), linter) }) From e1cb5c4df19a6f89f47374a2353c6a0b6f1ed812 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:04:38 +0800 Subject: [PATCH 04/16] needed extra path to handle file module function imports --- R/box_linters.R | 2 +- tests/testthat/test-linter_box_alphabetical.R | 36 ++++++++++++------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/R/box_linters.R b/R/box_linters.R index d41dd0fb..dd54e8a6 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -73,7 +73,7 @@ box_alphabetical_imports_linter <- function() { descendant::OP-LEFT-BRACKET ]") - xpath_functions <- "./expr/SYMBOL[ + xpath_functions <- "./descendant::expr/SYMBOL[ ../preceding-sibling::OP-LEFT-BRACKET and ../following-sibling::OP-RIGHT-BRACKET ]" diff --git a/tests/testthat/test-linter_box_alphabetical.R b/tests/testthat/test-linter_box_alphabetical.R index 04502387..57e433fe 100644 --- a/tests/testthat/test-linter_box_alphabetical.R +++ b/tests/testthat/test-linter_box_alphabetical.R @@ -106,22 +106,32 @@ test_that("box_alphabetical_imports_linter blocks unsorted imports in box::use() 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) + 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) + 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), - list(message = lint_message, line_number = 4) + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) ), linter) lintr::expect_lint(bad_box_calls_4, list( - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 4), - list(message = lint_message, line_number = 4) - ), linter) - lintr::expect_lint(bad_box_calls_5, list(message = lint_message), linter) - lintr::expect_lint(bad_box_calls_6, list(message = lint_message), linter) + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) + ), linter) + lintr::expect_lint(bad_box_calls_5, list( + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2) + ), linter) + lintr::expect_lint(bad_box_calls_6, list( + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) + ), linter) }) From e33c6410442c23951a32451c92fc77ddb9680540 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:08:31 +0800 Subject: [PATCH 05/16] dev version number increment --- DESCRIPTION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index af810683..c7e0496b 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: rhino Title: A Framework for Enterprise Shiny Applications -Version: 1.7.0.9005 +Version: 1.7.0.9001 Authors@R: c( person("Kamil", "Żyła", role = c("aut", "cre"), email = "opensource+kamil@appsilon.com"), From 9711b807e8eeb6a5ed1ff1450c27c219e7210113 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:08:48 +0800 Subject: [PATCH 06/16] add box_alphabetical_imports_filter to pkgdown --- pkgdown/_pkgdown.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/pkgdown/_pkgdown.yml b/pkgdown/_pkgdown.yml index ce926b57..a08e1f78 100644 --- a/pkgdown/_pkgdown.yml +++ b/pkgdown/_pkgdown.yml @@ -149,6 +149,7 @@ reference: - title: Linters contents: + - box_alphabetical_imports_linter - box_func_import_count_linter - box_separate_calls_linter - box_trailing_commas_linter From 5f0431e2dddd491aec87c0e703625c2517cec204 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:09:05 +0800 Subject: [PATCH 07/16] news entry --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) 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)_ From 11a585040c3221168b5c1aedbc81b580aa4c7e72 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:11:31 +0800 Subject: [PATCH 08/16] add new linter to .lintr template --- inst/templates/app_structure/dot.lintr | 1 + 1 file changed, 1 insertion(+) diff --git a/inst/templates/app_structure/dot.lintr b/inst/templates/app_structure/dot.lintr index fa3c8d97..4bee6dff 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_imports_linter = rhino::box_alphabetical_imports_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(), From 6a9e621b1a97311bb8355bc5b6193d9da6600c1b Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:22:42 +0800 Subject: [PATCH 09/16] delinting --- NAMESPACE | 2 +- R/box_linters.R | 42 +++++---- ...er.Rd => box_alphabetical_calls_linter.Rd} | 18 ++-- tests/testthat/test-linter_box_alphabetical.R | 89 +++++++++---------- 4 files changed, 74 insertions(+), 77 deletions(-) rename man/{box_alphabetical_imports_linter.Rd => box_alphabetical_calls_linter.Rd} (75%) diff --git a/NAMESPACE b/NAMESPACE index 7f08cdd7..392b78ac 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,7 +1,7 @@ # Generated by roxygen2: do not edit by hand export(app) -export(box_alphabetical_imports_linter) +export(box_alphabetical_calls_linter) export(box_func_import_count_linter) export(box_separate_calls_linter) export(box_trailing_commas_linter) diff --git a/R/box_linters.R b/R/box_linters.R index dd54e8a6..10981b35 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -11,22 +11,22 @@ #' # will produce lints #' lintr::lint( #' text = "box::use(packageB, packageA)", -#' linters = box_alphabetical_imports_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( #' text = "box::use(package[functionB, functionA])", -#' linters = box_func_import_count_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( #' text = "box::use(path/to/B, path/to/A)", -#' linters = box_alphabetical_imports_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( #' text = "box::use(path/to/A[functionB, functionA])", -#' linters = box_alphabetical_imports_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' # okay @@ -47,50 +47,50 @@ #' #' lintr::lint( #' text = "box::use(path/to/A, path/to/B)", -#' linters = box_alphabetical_imports_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( #' text = "box::use(path/to/A[functionA, functionB])", -#' linters = box_alphabetical_imports_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' @export # nolint end -box_alphabetical_imports_linter <- function() { - xpath_base <- "//SYMBOL_PACKAGE[(text() = 'box' and following-sibling::SYMBOL_FUNCTION_CALL[text() = 'use'])] +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_root <- xml2::xml_find_all(xml, xpath_base) 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], @@ -98,15 +98,15 @@ box_alphabetical_imports_linter <- function() { 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, @@ -114,13 +114,11 @@ box_alphabetical_imports_linter <- function() { type = "style" ) }) - + c(module_lint, function_lint) }) } - - # nolint start: line_length_linter #' `box` library function import count linter #' diff --git a/man/box_alphabetical_imports_linter.Rd b/man/box_alphabetical_calls_linter.Rd similarity index 75% rename from man/box_alphabetical_imports_linter.Rd rename to man/box_alphabetical_calls_linter.Rd index 384548d0..1fd8b757 100644 --- a/man/box_alphabetical_imports_linter.Rd +++ b/man/box_alphabetical_calls_linter.Rd @@ -1,10 +1,10 @@ % Generated by roxygen2: do not edit by hand % Please edit documentation in R/box_linters.R -\name{box_alphabetical_imports_linter} -\alias{box_alphabetical_imports_linter} +\name{box_alphabetical_calls_linter} +\alias{box_alphabetical_calls_linter} \title{\code{box} library alphabetical module and function imports linter} \usage{ -box_alphabetical_imports_linter() +box_alphabetical_calls_linter() } \value{ A custom linter function for use with \code{r-lib/lintr}. @@ -18,22 +18,22 @@ to learn about the details. # will produce lints lintr::lint( text = "box::use(packageB, packageA)", - linters = box_alphabetical_imports_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( text = "box::use(package[functionB, functionA])", - linters = box_func_import_count_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( text = "box::use(path/to/B, path/to/A)", - linters = box_alphabetical_imports_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( text = "box::use(path/to/A[functionB, functionA])", - linters = box_alphabetical_imports_linter() + linters = box_alphabetical_calls_linter() ) # okay @@ -54,12 +54,12 @@ lintr::lint( lintr::lint( text = "box::use(path/to/A, path/to/B)", - linters = box_alphabetical_imports_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( text = "box::use(path/to/A[functionA, functionB])", - linters = box_alphabetical_imports_linter() + linters = box_alphabetical_calls_linter() ) } diff --git a/tests/testthat/test-linter_box_alphabetical.R b/tests/testthat/test-linter_box_alphabetical.R index 57e433fe..c77804ed 100644 --- a/tests/testthat/test-linter_box_alphabetical.R +++ b/tests/testthat/test-linter_box_alphabetical.R @@ -1,6 +1,6 @@ -test_that("box_alphabetical_imports_linter skips allowed box::use() calls", { - linter <- box_alphabetical_imports_linter() - +test_that("box_alphabetical_calls_linter() skips allowed box::use() calls", { + linter <- box_alphabetical_calls_linter() + good_box_calls_1 <- "box::use( dplyr, shiny, @@ -12,7 +12,7 @@ test_that("box_alphabetical_imports_linter skips allowed box::use() calls", { path/to/fileB, path/to/fileC, )" - + good_box_calls_3 <- "box::use( dplyr[filter, mutate, select], shiny, @@ -31,107 +31,106 @@ test_that("box_alphabetical_imports_linter skips allowed box::use() calls", { lintr::expect_lint(good_box_calls_4, NULL, linter) }) -test_that("box_alphabetical_imports_linter ignores aliases in box::use() calls", { - linter <- box_alphabetical_imports_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_imports_linter blocks unsorted imports in box::use() call", { - linter <- box_alphabetical_imports_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.") - + + 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) + 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) - + 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), - list(message = lint_message, line_number = 4) - ), linter) + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) + ), linter) lintr::expect_lint(bad_box_calls_4, list( - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 4), - list(message = lint_message, line_number = 4) - ), linter) + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) + ), linter) lintr::expect_lint(bad_box_calls_5, list( - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 2) - ), linter) + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2) + ), linter) lintr::expect_lint(bad_box_calls_6, list( - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 4), - list(message = lint_message, line_number = 4) - ), linter) + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 2), + list(message = lint_message, line_number = 4), + list(message = lint_message, line_number = 4) + ), linter) }) From c5e2a745483ef321addce2da32582033361fe4d3 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:23:44 +0800 Subject: [PATCH 10/16] propagate changes caused by delinting --- inst/templates/app_structure/dot.lintr | 2 +- pkgdown/_pkgdown.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/inst/templates/app_structure/dot.lintr b/inst/templates/app_structure/dot.lintr index 4bee6dff..c285e634 100644 --- a/inst/templates/app_structure/dot.lintr +++ b/inst/templates/app_structure/dot.lintr @@ -1,7 +1,7 @@ linters: linters_with_defaults( line_length_linter = line_length_linter(100), - box_alphabetical_imports_linter = rhino::box_alphabetical_imports_linter(), + 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/pkgdown/_pkgdown.yml b/pkgdown/_pkgdown.yml index a08e1f78..c9bc81df 100644 --- a/pkgdown/_pkgdown.yml +++ b/pkgdown/_pkgdown.yml @@ -149,7 +149,7 @@ reference: - title: Linters contents: - - box_alphabetical_imports_linter + - box_alphabetical_calls_linter - box_func_import_count_linter - box_separate_calls_linter - box_trailing_commas_linter From 8600b4d1b7bca2c5d5a9d5f4ddb2791df2138bef Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 1 Mar 2024 17:39:03 +0800 Subject: [PATCH 11/16] add specific column numbers for lint --- tests/testthat/test-linter_box_alphabetical.R | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/testthat/test-linter_box_alphabetical.R b/tests/testthat/test-linter_box_alphabetical.R index c77804ed..e6ae9924 100644 --- a/tests/testthat/test-linter_box_alphabetical.R +++ b/tests/testthat/test-linter_box_alphabetical.R @@ -114,23 +114,23 @@ test_that("box_alphabetical_calls_linter() blocks unsorted imports in box::use() list(message = lint_message, line_number = 4) ), linter) lintr::expect_lint(bad_box_calls_3, list( - list(message = lint_message, line_number = 4), - list(message = lint_message, line_number = 4) + 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), - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 4), - list(message = lint_message, line_number = 4) + 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), - list(message = lint_message, line_number = 2) + 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), - list(message = lint_message, line_number = 2), - list(message = lint_message, line_number = 4), - list(message = lint_message, line_number = 4) + 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) }) From db899a1de3254d44d6d3f32e05a5071445b65169 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 8 Mar 2024 14:31:40 +0800 Subject: [PATCH 12/16] added box alias handling (or ignoring it) to documentation --- R/box_linters.R | 19 +++++++++++++++---- man/box_alphabetical_calls_linter.Rd | 19 +++++++++++++++---- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/R/box_linters.R b/R/box_linters.R index 10981b35..b1de7ff3 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -1,7 +1,8 @@ # nolint start: line_length_linter #' `box` library alphabetical module and function imports linter #' -#' Checks that module and function imports are sorted alphabetically. +#' 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. #' @@ -29,20 +30,25 @@ #' 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_func_import_count_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( #' text = "box::use(package[one, two, three])", -#' linters = box_func_import_count_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( #' text = "box::use(package[functionA, functionB])", -#' linters = box_func_import_count_linter() +#' linters = box_alphabetical_calls_linter() #' ) #' #' lintr::lint( @@ -55,6 +61,11 @@ #' 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() { diff --git a/man/box_alphabetical_calls_linter.Rd b/man/box_alphabetical_calls_linter.Rd index 1fd8b757..09d9396f 100644 --- a/man/box_alphabetical_calls_linter.Rd +++ b/man/box_alphabetical_calls_linter.Rd @@ -10,7 +10,8 @@ box_alphabetical_calls_linter() A custom linter function for use with \code{r-lib/lintr}. } \description{ -Checks that module and function imports are sorted alphabetically. +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. } @@ -36,20 +37,25 @@ lintr::lint( 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_func_import_count_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( text = "box::use(package[one, two, three])", - linters = box_func_import_count_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( text = "box::use(package[functionA, functionB])", - linters = box_func_import_count_linter() + linters = box_alphabetical_calls_linter() ) lintr::lint( @@ -62,4 +68,9 @@ lintr::lint( linters = box_alphabetical_calls_linter() ) +lintr::lint( + text = "box::use(path/to/A[functionA, alias = functionB])", + linters = box_alphabetical_calls_linter() +) + } From e6e83bd06082473f4afb238fe273d194e78d61e1 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 8 Mar 2024 14:35:43 +0800 Subject: [PATCH 13/16] updated box-modules vignette for consistency --- vignettes/explanation/box-modules.Rmd | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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], ) From 5e4d5b0c7ba438aaf95efe5042c1a94dab72aee9 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 8 Mar 2024 16:16:48 +0800 Subject: [PATCH 14/16] indent xpath --- R/box_linters.R | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/R/box_linters.R b/R/box_linters.R index b1de7ff3..55892487 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -70,25 +70,25 @@ # 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" + following-sibling::SYMBOL_FUNCTION_CALL[text() = 'use'])] + /parent::expr + /parent::expr" xpath <- paste(xpath_base, " -/child::expr[ - descendant::SYMBOL -]") + /child::expr[ + descendant::SYMBOL + ]") xpath_modules_with_functions <- paste(xpath_base, " -/child::expr[ - descendant::SYMBOL and - descendant::OP-LEFT-BRACKET -]") + /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 -]" + ../preceding-sibling::OP-LEFT-BRACKET and + ../following-sibling::OP-RIGHT-BRACKET + ]" lint_message <- "Module and function imports must be sorted alphabetically." From a84f9758ba37c228425a220e500b9ebe91a76747 Mon Sep 17 00:00:00 2001 From: Rodrigo Basa <rodrigo@appsilon.com> Date: Fri, 8 Mar 2024 16:17:05 +0800 Subject: [PATCH 15/16] add sub section on order of imports and aliases --- vignettes/explanation/rhino-style-guide.Rmd | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/vignettes/explanation/rhino-style-guide.Rmd b/vignettes/explanation/rhino-style-guide.Rmd index a6914789..f59a425f 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. From 795899912c9a9701ca9b61eb19bf60fbc5bc1ee5 Mon Sep 17 00:00:00 2001 From: Jakub Nowicki <kuba@appsilon.com> Date: Fri, 8 Mar 2024 14:35:45 +0100 Subject: [PATCH 16/16] chore: Minor documentation fixes. --- R/box_linters.R | 9 ++------- man/box_alphabetical_calls_linter.Rd | 5 ----- vignettes/explanation/rhino-style-guide.Rmd | 2 +- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/R/box_linters.R b/R/box_linters.R index 55892487..987884ff 100644 --- a/R/box_linters.R +++ b/R/box_linters.R @@ -42,15 +42,10 @@ #' ) #' #' lintr::lint( -#' text = "box::use(package[one, two, three])", -#' 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() @@ -69,7 +64,7 @@ #' @export # nolint end box_alphabetical_calls_linter <- function() { - xpath_base <- "//SYMBOL_PACKAGE[(text() = 'box' and + xpath_base <- "//SYMBOL_PACKAGE[(text() = 'box' and following-sibling::SYMBOL_FUNCTION_CALL[text() = 'use'])] /parent::expr /parent::expr" diff --git a/man/box_alphabetical_calls_linter.Rd b/man/box_alphabetical_calls_linter.Rd index 09d9396f..6010a604 100644 --- a/man/box_alphabetical_calls_linter.Rd +++ b/man/box_alphabetical_calls_linter.Rd @@ -48,11 +48,6 @@ lintr::lint( linters = box_alphabetical_calls_linter() ) -lintr::lint( - text = "box::use(package[one, two, three])", - linters = box_alphabetical_calls_linter() -) - lintr::lint( text = "box::use(package[functionA, functionB])", linters = box_alphabetical_calls_linter() diff --git a/vignettes/explanation/rhino-style-guide.Rmd b/vignettes/explanation/rhino-style-guide.Rmd index f59a425f..393410bc 100644 --- a/vignettes/explanation/rhino-style-guide.Rmd +++ b/vignettes/explanation/rhino-style-guide.Rmd @@ -104,7 +104,7 @@ box::use( # Bad box::use( a_pkg = shiny, - rhino[a_fun = react_component, log] + rhino[a_fun = react_component, log], ) ```