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.

Sign up for GitHub

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

Mention the specific operator in lint message for vector_logic_linter() #2398

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Mention the specific operator in lint message for vector_logic_linter() #2398

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
953c922
mention the specific operator in lint message
MichaelChirico Dec 6, 2023
74d7f64
use strrep
MichaelChirico Dec 6, 2023
f42a474
Merge branch 'main' into vec-op
AshesITR Dec 6, 2023
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
5 changes: 4 additions & 1 deletion R/vector_logic_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,18 @@ vector_logic_linter <- function() {
]
"

scalar_map <- c(AND = "&&", OR = "||")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could avoid this map by pasting the op text twice instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that feels over-optimized to me, vector look-up map is very fast in general. If anything I'd do strrep(xml_text(bad_expr), 2L) instead, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, the map approach is way faster than sprintf(), and only slower than strrep() in extreme cases where there's a huge # of lints in the same expression:

op_and = "AND"
op_or = "||"
both = sample(c(op_and, op_or), 1e3, TRUE)
map = c(AND = "&&", or = "||")

str_and = "&"
str_or = "|"
str_both = sample(c(str_or, str_and), 1e3, TRUE)

microbenchmark(times = 1e4,
  map[op_and], sprintf("%1$s%1$s", str_and), strrep(str_and, 2L),
  map[op_or], sprintf("%1$s%1$s", str_or), strrep(str_or, 2L),
  map[both], sprintf("%1$s%1$s", str_both), strrep(str_both, 2L))
Unit: nanoseconds
                          expr    min     lq        mean median       uq      max neval cld
                   map[op_and]    421    521    845.0018    581    721.0    98771 10000 a  
  sprintf("%1$s%1$s", str_and)    762    861   1343.1966    941   1150.0   111471 10000 a  
           strrep(str_and, 2L)    710    812   1307.5081    901   1111.0    99381 10000 a  
                    map[op_or]    591    730   2214.5031    831   1061.0 10363474 10000 a  
   sprintf("%1$s%1$s", str_or)    770    870   1330.1354    941   1150.0    98651 10000 a  
            strrep(str_or, 2L)    710    821   1318.9603    901   1121.0    61772 10000 a  
                     map[both]  45481  49541  68498.7497  52581  58176.0 10830374 10000  b 
 sprintf("%1$s%1$s", str_both) 125141 126591 141180.6610 127491 134721.5 10666224 10000   c
          strrep(str_both, 2L)  52001  55821  64786.3580  56250  60006.5 10300914 10000  b

I find the look-up approach perfectly readable, but maybe strrep() is about the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark is only valid if you copy the full lint message into the map 😉
Anyway, I'd prefer a self-contained approach where all parts of the lint message are assembled in the same place, either outside or inside the linter function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to strrep().


Linter(linter_level = "expression", function(source_expression) {
xml <- source_expression$xml_parsed_content
if (is.null(xml)) return(list())
bad_expr <- xml_find_all(xml, xpath)

op <- xml_name(bad_expr)
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
xml_nodes_to_lints(
bad_expr,
source_expression = source_expression,
lint_message = "Conditional expressions require scalar logical operators (&& and ||)",
lint_message = sprintf("Use `%s` in conditional expressions.", scalar_map[op]),
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
type = "warning"
)
})
Expand Down
29 changes: 18 additions & 11 deletions tests/testthat/test-vector_logic_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,37 @@ test_that("vector_logic_linter skips allowed usages", {

test_that("vector_logic_linter blocks simple disallowed usages", {
linter <- vector_logic_linter()
lint_msg <- rex::rex("Conditional expressions require scalar logical operators")

expect_lint("if (TRUE & FALSE) 1", lint_msg, linter)
expect_lint("while (TRUE | TRUE) 2", lint_msg, linter)
expect_lint("if (TRUE & FALSE) 1", rex::rex("Use `&&` in conditional expressions."), linter)
expect_lint("while (TRUE | TRUE) 2", rex::rex("Use `||` in conditional expressions."), linter)
})

test_that("vector_logic_linter detects nested conditions", {
linter <- vector_logic_linter()
lint_msg <- rex::rex("Conditional expressions require scalar logical operators")

expect_lint("if (TRUE & TRUE || FALSE) 4", lint_msg, linter)
expect_lint("if (TRUE && (TRUE | FALSE)) 4", lint_msg, linter)
expect_lint(
"if (TRUE & TRUE || FALSE) 4",
list(rex::rex("Use `&&` in conditional expressions."), column_number = 10L),
linter
)
expect_lint(
"if (TRUE && (TRUE | FALSE)) 4",
list(rex::rex("Use `||` in conditional expressions."), column_number = 19L),
linter
)
})

test_that("vector_logic_linter catches usages in expect_true()/expect_false()", {
linter <- vector_logic_linter()
lint_msg <- rex::rex("Conditional expressions require scalar logical operators")
and_msg <- rex::rex("Use `&&` in conditional expressions.")
or_msg <- rex::rex("Use `||` in conditional expressions.")

expect_lint("expect_true(TRUE & FALSE)", lint_msg, linter)
expect_lint("expect_false(TRUE | TRUE)", lint_msg, linter)
expect_lint("expect_true(TRUE & FALSE)", and_msg, linter)
expect_lint("expect_false(TRUE | TRUE)", or_msg, linter)

# ditto with namespace qualification
expect_lint("testthat::expect_true(TRUE & FALSE)", lint_msg, linter)
expect_lint("testthat::expect_false(TRUE | TRUE)", lint_msg, linter)
expect_lint("testthat::expect_true(TRUE & FALSE)", and_msg, linter)
expect_lint("testthat::expect_false(TRUE | TRUE)", or_msg, linter)
})

test_that("vector_logic_linter doesn't get mixed up from complex usage", {
Expand Down
Loading