Skip to content

Extend boolean_arithmetic_linter to detect all()-like cases too #1581

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

Open
MichaelChirico opened this issue Sep 30, 2022 · 1 comment
Open
Labels
feature a feature request or enhancement

Comments

@MichaelChirico
Copy link
Collaborator

Follow-up to #1579

Extend boolean_arithmetic_linter() to include all()-alike expressions like sum(x == y) == length(x) --> all(x == y).

The issues are

  1. we can't just test for length() on the RHS (e.g. sum(x == y) == length(z) could be totally different)
  2. looking for sum(x == y) == length(x == y) (i.e., exact expression matching, which we do in some other places) will only find a particularly silly manifestation of the issue
  3. there is a bit of a combinatorial explosion if we have to look for any symbol found inside the logical expression on the RHS (e.g. sum(x == y) == [length(x) _or_ length(y)]).

A first pass would be to check how many hits there are for the naive version (i.e., condition 1), to see how worth investing in this complicated logic is to begin with.

@MichaelChirico MichaelChirico added the false-negative code that should lint, but doesn't label Sep 30, 2022
@MichaelChirico
Copy link
Collaborator Author

Adding potential test cases:

test_that("boolean_arithmetic_linter blocks comparisons to the object length", {
  expect_lint(
    "length(which(condition)) < length(condition)",
    "grep\\(pattern, x\\) is better than which\\(grepl\\(pattern, x\\)\\)\\.",
    boolean_arithmetic_linter
  )
  expect_lint(
    "length(which(is_treatment)) == length(is_treatment)",
    "grep\\(pattern, x\\) is better than which\\(grepl\\(pattern, x\\)\\)\\.",
    boolean_arithmetic_linter
  )
  expect_lint(
    "length(grep(pattern, x)) < length(x)",
    "grep\\(pattern, x\\) is better than which\\(grepl\\(pattern, x\\)\\)\\.",
    boolean_arithmetic_linter
  )
  expect_lint(
    "sum(x == y) < length(x)",
    "grep\\(pattern, x\\) is better than which\\(grepl\\(pattern, x\\)\\)\\.",
    boolean_arithmetic_linter
  )
  expect_lint(
    "sum(grepl(pattern, x)) == length(x)",
    "grep\\(pattern, x\\) is better than which\\(grepl\\(pattern, x\\)\\)\\.",
    boolean_arithmetic_linter
  )
})

MichaelChirico added a commit that referenced this issue Sep 30, 2022
IndrajeetPatil added a commit that referenced this issue Oct 2, 2022
* New boolean_arithmetic_linter

* follow-up #1580

* Follow-up #1581

* Update R/boolean_arithmetic_linter.R

* update docs and linter counts

Co-authored-by: Indrajeet Patil <[email protected]>
@MichaelChirico MichaelChirico added feature a feature request or enhancement and removed false-negative code that should lint, but doesn't labels Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

1 participant