From 3e6b862da57817a488bc8b263d31ac17b4f99de6 Mon Sep 17 00:00:00 2001 From: Nicholas Masel <61123199+nicholas-masel@users.noreply.github.com> Date: Thu, 3 Aug 2023 19:55:30 -0400 Subject: [PATCH] Add library call linter (#2027) * add library call linter * tests * correct lint findings * fix lint results * return last node rather than last element * remove debug code * review comments * add explicit L for integers * indent XPath one more level * add readability * added news --------- Co-authored-by: Nicholas Masel Co-authored-by: AshesITR Co-authored-by: Michael Chirico --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 5 ++ R/library_call_linter.R | 75 +++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/library_call_linter.Rd | 56 +++++++++++++ man/linters.Rd | 7 +- man/readability_linters.Rd | 1 + man/style_linters.Rd | 1 + tests/testthat/test-library_call_linter.R | 99 +++++++++++++++++++++++ 11 files changed, 245 insertions(+), 3 deletions(-) create mode 100644 R/library_call_linter.R create mode 100644 man/library_call_linter.Rd create mode 100644 tests/testthat/test-library_call_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 0358b669f..c9feea16b 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -114,6 +114,7 @@ Collate: 'is_lint_level.R' 'is_numeric_linter.R' 'lengths_linter.R' + 'library_call_linter.R' 'line_length_linter.R' 'lint.R' 'linter_tag_docs.R' diff --git a/NAMESPACE b/NAMESPACE index 43a3eba8c..3bba76ee7 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -78,6 +78,7 @@ export(inner_combine_linter) export(is_lint_level) export(is_numeric_linter) export(lengths_linter) +export(library_call_linter) export(line_length_linter) export(lint) export(lint_dir) diff --git a/NEWS.md b/NEWS.md index 63322d2f4..cbb36398e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,11 @@ * `inner_combine_linter()` no longer throws on length-1 calls to `c()` like `c(exp(2))` or `c(log(3))` (#2017, @MichaelChirico). Such usage is discouraged by `unnecessary_concatenation_linter()`, but `inner_combine_linter()` _per se_ does not apply. +## New and improved features + +* `library_call_linter()` can detect if all library calls are not at the top of your script (#2027, @nicholas-masel). + + ## Changes to defaults * `assignment_linter()` lints the {magrittr} assignment pipe `%<>%` (#2008, @MichaelChirico). This can be deactivated by setting the new argument `allow_pipe_assign` to `TRUE`. diff --git a/R/library_call_linter.R b/R/library_call_linter.R new file mode 100644 index 000000000..52411f8fe --- /dev/null +++ b/R/library_call_linter.R @@ -0,0 +1,75 @@ +#' Library call linter +#' +#' Force library calls to all be at the top of the script. +#' +#' @examples +#' # will produce lints +#' lint( +#' text = " +#' library(dplyr) +#' print('test') +#' library(tidyr) +#' ", +#' linters = library_call_linter() +#' ) +#' +#' lint( +#' text = " +#' library(dplyr) +#' print('test') +#' library(tidyr) +#' library(purrr) +#' ", +#' linters = library_call_linter() +#' ) +#' +#' # okay +#' lint( +#' text = " +#' library(dplyr) +#' print('test') +#' ", +#' linters = library_call_linter() +#' ) +#' +#' lint( +#' text = " +#' # comment +#' library(dplyr) +#' ", +#' linters = library_call_linter() +#' ) +#' +#' @evalRd rd_tags("library_call_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +library_call_linter <- function() { + + xpath <- " + (//SYMBOL_FUNCTION_CALL[text() = 'library'])[last()] + /preceding::expr + /SYMBOL_FUNCTION_CALL[text() != 'library'][last()] + /following::expr[SYMBOL_FUNCTION_CALL[text() = 'library']] + " + + Linter(function(source_expression) { + if (!is_lint_level(source_expression, "file")) { + return(list()) + } + + xml <- source_expression$full_xml_parsed_content + + bad_expr <- xml2::xml_find_all(xml, xpath) + + if (length(bad_expr) == 0L) { + return(list()) + } + + xml_nodes_to_lints( + bad_expr, + source_expression = source_expression, + lint_message = "Move all library calls to the top of the script.", + type = "warning" + ) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index e6bbebaed..ba873d4cb 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -42,6 +42,7 @@ infix_spaces_linter,style readability default configurable inner_combine_linter,efficiency consistency readability is_numeric_linter,readability best_practices consistency lengths_linter,efficiency readability best_practices +library_call_linter,style best_practices readability line_length_linter,style readability default configurable literal_coercion_linter,best_practices consistency efficiency matrix_apply_linter,readability efficiency diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 397b8e9c2..7223e44ed 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -41,6 +41,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{implicit_integer_linter}}} \item{\code{\link{is_numeric_linter}}} \item{\code{\link{lengths_linter}}} +\item{\code{\link{library_call_linter}}} \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{outer_negation_linter}}} diff --git a/man/library_call_linter.Rd b/man/library_call_linter.Rd new file mode 100644 index 000000000..2ee4260e5 --- /dev/null +++ b/man/library_call_linter.Rd @@ -0,0 +1,56 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/library_call_linter.R +\name{library_call_linter} +\alias{library_call_linter} +\title{Library call linter} +\usage{ +library_call_linter() +} +\description{ +Force library calls to all be at the top of the script. +} +\examples{ +# will produce lints +lint( + text = " + library(dplyr) + print('test') + library(tidyr) + ", + linters = library_call_linter() +) + +lint( + text = " + library(dplyr) + print('test') + library(tidyr) + library(purrr) + ", + linters = library_call_linter() +) + +# okay +lint( + text = " + library(dplyr) + print('test') + ", + linters = library_call_linter() +) + +lint( + text = " + # comment + library(dplyr) + ", + linters = library_call_linter() +) + +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=readability_linters]{readability}, \link[=style_linters]{style} +} diff --git a/man/linters.Rd b/man/linters.Rd index 8cce38cf6..cf75146cc 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,7 +17,7 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (50 linters)} +\item{\link[=best_practices_linters]{best_practices} (51 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (7 linters)} \item{\link[=configurable_linters]{configurable} (29 linters)} \item{\link[=consistency_linters]{consistency} (18 linters)} @@ -27,9 +27,9 @@ The following tags exist: \item{\link[=efficiency_linters]{efficiency} (23 linters)} \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} -\item{\link[=readability_linters]{readability} (47 linters)} +\item{\link[=readability_linters]{readability} (48 linters)} \item{\link[=robustness_linters]{robustness} (14 linters)} -\item{\link[=style_linters]{style} (34 linters)} +\item{\link[=style_linters]{style} (35 linters)} } } \section{Linters}{ @@ -76,6 +76,7 @@ The following linters exist: \item{\code{\link{inner_combine_linter}} (tags: consistency, efficiency, readability)} \item{\code{\link{is_numeric_linter}} (tags: best_practices, consistency, readability)} \item{\code{\link{lengths_linter}} (tags: best_practices, efficiency, readability)} +\item{\code{\link{library_call_linter}} (tags: best_practices, readability, style)} \item{\code{\link{line_length_linter}} (tags: configurable, default, readability, style)} \item{\code{\link{literal_coercion_linter}} (tags: best_practices, consistency, efficiency)} \item{\code{\link{matrix_apply_linter}} (tags: efficiency, readability)} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index d1f32efec..9a3979583 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -34,6 +34,7 @@ The following linters are tagged with 'readability': \item{\code{\link{inner_combine_linter}}} \item{\code{\link{is_numeric_linter}}} \item{\code{\link{lengths_linter}}} +\item{\code{\link{library_call_linter}}} \item{\code{\link{line_length_linter}}} \item{\code{\link{matrix_apply_linter}}} \item{\code{\link{nested_ifelse_linter}}} diff --git a/man/style_linters.Rd b/man/style_linters.Rd index 1c4e93c17..a3f8a8c67 100644 --- a/man/style_linters.Rd +++ b/man/style_linters.Rd @@ -25,6 +25,7 @@ The following linters are tagged with 'style': \item{\code{\link{implicit_integer_linter}}} \item{\code{\link{indentation_linter}}} \item{\code{\link{infix_spaces_linter}}} +\item{\code{\link{library_call_linter}}} \item{\code{\link{line_length_linter}}} \item{\code{\link{numeric_leading_zero_linter}}} \item{\code{\link{object_length_linter}}} diff --git a/tests/testthat/test-library_call_linter.R b/tests/testthat/test-library_call_linter.R new file mode 100644 index 000000000..8e2184f1e --- /dev/null +++ b/tests/testthat/test-library_call_linter.R @@ -0,0 +1,99 @@ +test_that("library_call_linter skips allowed usages", { + + expect_lint( + trim_some(" + library(dplyr) + print('test') + "), + NULL, + library_call_linter() + ) + + expect_lint("print('test')", + NULL, + library_call_linter() + ) + + expect_lint( + trim_some(" + # comment + library(dplyr) + "), + NULL, + library_call_linter() + ) + + expect_lint( + trim_some(" + print('test') + # library(dplyr) + "), + NULL, + library_call_linter() + ) +}) + +test_that("library_call_linter warns on disallowed usages", { + lint_message <- rex::rex("Move all library calls to the top of the script.") + + expect_lint( + trim_some(" + library(dplyr) + print('test') + library(tidyr) + "), + lint_message, + library_call_linter() + ) + + expect_lint( + trim_some(" + library(dplyr) + print('test') + library(tidyr) + library(purrr) + "), + list( + list(lint_message, line_number = 3L, column_number = 1L), + list(lint_message, line_number = 4L, column_number = 1L) + ), + library_call_linter() + ) + + expect_lint( + trim_some(" + library(dplyr) + print('test') + print('test') + library(tidyr) + "), + lint_message, + library_call_linter() + ) + + expect_lint( + trim_some(" + library(dplyr) + print('test') + library(tidyr) + print('test') + "), + lint_message, + library_call_linter() + ) + + expect_lint( + trim_some(" + library(dplyr) + print('test') + library(tidyr) + print('test') + library(purrr) + "), + list( + list(lint_message, line_number = 3L, column_number = 1L), + list(lint_message, line_number = 5L, column_number = 1L) + ), + library_call_linter() + ) +})