Closed
Description
Originally posted by @MichaelChirico in #1311 (comment)
My instinct is we should prefer the actionable bit up front, with explanations in the message only for clarification in cases where we can't provide laser-focused alternatives/diagnosis. The thinking being if there are dozens of lints on a file / package, the output will be easier to read/delint if the user can snap to the beginning of each message to find their action item.
OTOH I think we're a bit inconsistent on this front overall. Here are the ~120 existing messages produced by our test suite (excluding error lints, and lightly edited to condense things a bit):
<<1 | 1L>>:<<dim(...)[1L] | dim(...)[[1L]] | length | ncol | nrow | NCOL | NROW>> is likely to be wrong in the empty edge case. Use <<seq_len | seq_along>>() instead.
!all(x) is better than any(!x). The former applies negation only once after aggregation instead of many times for each element of x.
anyDuplicated(DF$col) == 0L is better than length(unique(DF$col)) == nrow(DF)
anyDuplicated(x, ...) > 0 is better than any(duplicated(x), ...).
anyDuplicated(x) == 0L is better than length(unique(x)) == length(x).
Any function spanning multiple lines should use curly braces.
anyNA(x) is better than any(is.na(x)).
!any(x) is better than all(!x). The former applies negation only once after aggregation instead of many times for each element of x.
Avoid storing placeholder tests like expect_equal(1, 1)
<< <<- | ->> >> can have hard-to-predict behavior; prefer assigning to a specific environment instead (with assign() or <-).
Closing curly-braces should always be on their own line, unless they are followed by an else.
Code and comments coming after a top-level return() or stop() should be removed.
Combine inputs to vectorized functions first to take full advantage of vectorization, e.g., %s(c(x, y)) only runs the more expensive %s() once as compared to c(%s(x), %s(y)).
Commas should always have a space after.
Commas should never have a space before.
Commented code should be removed.
Compound semicolons are discouraged. Replace them by a newline.
Conditional expressions require scalar logical operators (&& and ||)
Do not place spaces after parentheses.
Do not place spaces after square brackets.
Do not place spaces before parentheses.
Do not place spaces before square brackets.
Do not use absolute paths.
Don't alter the search() path in <<.onAttach | .onLoad>>() by calling <<library | require>>().
Don't slow down package load by running installed.packages() in .onLoad().
Don't use cat() in .onLoad().
Don't use <<F | T>> as a variable name, as it can break code relying on <<F | T>> being <<FALSE | TRUE>>.
Don't use message() in .onAttach().
Don't use nested <<ifelse | fifelse | if_else.>() calls; instead, try (1) data.table::fcase; (2) dplyr::case_when; or (3) using a lookup table.
Don't use <<paste | paste0>> to build <<stop | warning | message | packageStartupMessage>> strings. Instead use the fact that these functions build condition message strings from their input (using "" as a separator). For translateable strings, prefer using gettextf().
Don't use print() in .onLoad().
Don't use writeLines() in .onAttach().
Duplicate arguments in function call.
Either both or neither branch in `if`/`else` should use curly braces.
`else` should come on the same line as the previous `}`.
expect_false(x) is better than expect_identical(x, FALSE)
expect_false(x) is better than expect_true(!x), and vice versa.
expect_gte(x, y) is better than expect_true(x >= y).
expect_gt(x, y) is better than expect_true(x > y).
expect_identical(x, y) is better than expect_true(x == y).
expect_length(x, n) is better than <<expect_equal | expect_identical>>(length(x), n)
expect_lte(x, y) is better than expect_true(x <= y).
expect_lt(x, y) is better than expect_true(x < y).
expect_named(x, n) is better than <<expect_equal | expect_identical>>(names(x), n)
expect_null(x) is better than <<expect_equal | expect_identical>><<(x, NULL) | (is.null(x))>>
expect_s3_class(x, k) is better than expect_equal(class(x), k). Note also expect_s4_class() available for testing S4 objects.
expect_s3_class(x, k) is better than expect_identical(class(x), k). Note also expect_s4_class() available for testing S4 objects.
expect_s3_class(x, k) is better than expect_true(is.<k>(x)) or expect_true(inherits(x, k)). Note also expect_s4_class() available for testing S4 objects.
expect_s4_class(x, k) is better than expect_true(is(x, k)). Note also expect_s3_class() available for testing S3 objects.
expect_true(x) is better than expect_equal(x, TRUE)
expect_type(x, t) is better than expect_equal(typeof(x), t)
expect_type(x, t) is better than expect_identical(typeof(x), t)
expect_type(x, t) is better than expect_true(is.<t>(x))
For static regular expression patterns, set `fixed = TRUE`. Note that this includes regular expressions that can be expressed as fixed patterns, e.g. [.] is really just . and \$ is really just $ if there are no other regular expression specials. For functions from the 'stringr' package, the way to declare a static string is to wrap the pattern in stringr::fixed(). If this is being used in a dbplyr context (i.e., translated to sql), replace the regular expression with the `LIKE` operator using the `%LIKE%` infix function. Lastly, take care to remember that the `replacement` argument of `gsub()` is affected by the `fixed` argument as well.
Function "%s" is undesirable.
Functions should have cyclomatic complexity of less than %d, this has %d.
Include the leading zero for fractional numeric constants.
Instead of comparing class(x) with <<%in% | == | !=>>, use inherits(x, 'class-name') or is.<class> or is(x, 'class')
Instead of <<assert_that | expect_false | expect_true | stopifnot>>(A || B), write multiple expectations like %s(A) and %s(B) The latter will produce better error messages in the case of failure.
Integers should not be implicit. Use the form 1L for integers or 1.0 for doubles.
Just use the logical condition (or its negation) directly instead of calling <<ifelse | fifelse | if_else>>fifelse(x, %s, %s)
.Last.lib() should take one argument starting with 'lib'.
length(...):1 is likely to be wrong in the empty edge case. Use seq_along() instead.
Lines should not be more than %d characters.
local variable '%s' assigned but may not be used
Missing argument in function call.
Missing terminal newline.
no visible <<binding for global variable | global function definition>> '%s'
.onAttach() should take two arguments, with the first starting with 'lib' and the second starting with 'pkg'.
.onDetach() should take one argument starting with 'lib'.
.onLoad() should take two arguments, with the first starting with 'lib' and the second starting with 'pkg'.
Only use double-quotes.
Opening curly braces should <<always be followed by a new line... | never go on their own line...>>.
Operator `%s` is undesirable. << As an alternative... | >>
Package '%s' is <<attached but never used | only used by namespace...>>.
paste0(...) is better than paste(..., sep = "").
Place a space before left parenthesis, except in a function call.
<<pmin | pmax>>(x, y) is preferable to <<ifelse | fifelse | if_else>(x %s y, %s, %s).
Prefer as.numeric(x) to <<ifelse | fifelse | if_else>>(x, %d, %d) if really needed, but do note that R will usually convert logical vectors to 0/1 on the fly when needed.
Prefer grep(pattern, x, ..., value = TRUE) over x[grep(pattern, x, ...)] and x[grepl(pattern, x, ...)].
Prefer stringr::str_subset(x, pattern) over x[str_detect(x, pattern)] and x[str_which(x, pattern)].
Put exactly one space on each side of infix operators.
Put library.dynam() calls in .onLoad, not .onAttach().
Put packageStartupMessage() calls in .onAttach(), not .onLoad().
Put spaces around all infix operators.
Remove spaces before the left parenthesis in a function call.
'%s' <<does not exist | is not exported from>> in {%s}.
'%s' is exported from {%s}. Use %s::%s instead.
sep= is not a formal argument to paste0(); did you mean to use paste(), or collapse=?
`%>%` should always have a space before it and a new line after it, unless the full pipeline fits on one line.
Tests should compare objects in the order 'actual', 'expected', not the reverse. For example, do %s(foo(x), 2L) instead of %s(2L, foo(x)).
There should be a space before an opening curly brace.
There should be a space between right parenthesis and a body expression.
There should be a space between right parenthesis and an opening curly brace.
This code relies on the default value of stringsAsFactors, which changed in version R 4.0. Please supply an explicit value for stringsAsFactors for this code to work with versions of R both before and after this switch.
TODO comments should be removed.
toString(.) is more expressive than paste(., collapse = ", "). Note also glue::glue_collapse() and and::and() for constructing human-readable / translation-friendly lists
Trailing blank lines are superfluous.
Trailing semicolons are not needed.
Trailing whitespace is superfluous.
%s (R %s) is not available for dependency R >= %s.
Unify consecutive calls to stopifnot().
Unneeded concatenation of a constant. Remove the "c" call.
Unneeded concatenation without arguments. Replace the "c" call by NULL or, whenever possible, vector() seeded with the correct type and/or length.
Use expect_identical(x, y) by default; resort to expect_equal() only when needed, e.g. when setting ignore_attr= or tolerance=.
Use explicit calls in magrittr pipes, i.e., `a %>% foo` should be `a %>% foo()`.
Use file.path() to construct portable file paths.
Use `[[` instead of `%s` to extract an element.
Use is.na for comparisons to NA (not == or !=)
Use library.dynam.unload() calls in .onUnload(), not .Last.lib().
Use library.dynam.unload() calls in .onUnload(), not .onDetach().
Use literals directly where possible, instead of coercion. c.f. 1L instead of as.integer(1), or NA_real_ instead of as.numeric(NA).
Use <-, not %s, for assignment.
Use spaces to indent, not tabs.
Use <<startsWith() | endsWith()>> to detect <<a fixed initial | an initial | a fixed terminal | a terminal>> substring.
Use the `...` argument of system.file() to expand paths, e.g. system.file("data", "model.csv", package = "myrf") instead of <<file.path(system.file(...)) | system.file(file.path(...))>>
Use <<TRUE | FALSE>> instead of the symbol <<T | F>>.
Variable and function names should not be longer than %d characters.
Variable and function name style should be %s.
I think "action, reason" is more common than "reason, action".
WDYT?