Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Box alphabetical imports linter. #544

Merged
merged 17 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -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 = "[email protected]"),
Expand Down
1 change: 1 addition & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -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)_
Expand Down
132 changes: 132 additions & 0 deletions R/box_linters.R
Original file line number Diff line number Diff line change
@@ -1,3 +1,135 @@
# 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)
jakubnowicki marked this conversation as resolved.
Show resolved Hide resolved
#' to learn about the details.
#'
#' @return A custom linter function for use with `r-lib/lintr`.
#'
#' @examples
jakubnowicki marked this conversation as resolved.
Show resolved Hide resolved
#' # 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[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()
#' )
#'
#' 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
]"
radbasa marked this conversation as resolved.
Show resolved Hide resolved

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
#'
Expand Down
1 change: 1 addition & 0 deletions inst/templates/app_structure/dot.lintr
Original file line number Diff line number Diff line change
@@ -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(),
Expand Down
76 changes: 76 additions & 0 deletions man/box_alphabetical_calls_linter.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkgdown/_pkgdown.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
136 changes: 136 additions & 0 deletions tests/testthat/test-linter_box_alphabetical.R
radbasa marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -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],
)"

jakubnowicki marked this conversation as resolved.
Show resolved Hide resolved
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)
})
Loading
Loading