Skip to content

expect_comparison_linter() gets bad metadata for faulty expect_true() usage #2083

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

Closed
MichaelChirico opened this issue Aug 13, 2023 · 2 comments · Fixed by #2408
Closed

expect_comparison_linter() gets bad metadata for faulty expect_true() usage #2083

MichaelChirico opened this issue Aug 13, 2023 · 2 comments · Fixed by #2408
Milestone

Comments

@MichaelChirico
Copy link
Collaborator

Came across this:

expect_true(is.count(n_draws), n_draws > 1)

This throws expect_comparison_linter(), but gives a bad message:

lint(
  text = "expect_true(is.count(n_draws), n_draws > 1)",
  linters = expect_comparison_linter()
)
<text>:1:1: warning: [expect_comparison_linter] NA(x, y) is better than expect_true(x ( y).
expect_true(is.count(n_draws), n_draws > 1)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Maybe we shouldn't invest in doing anything here since expect_true()'s been used incorrectly, but certainly it's not a helpful lint message.

@AshesITR
Copy link
Collaborator

Can the bug be triggered by a valid usage like

expect_true(info = "my info first", a > b)

?

If so, I think putting a more robust XPath in place to find the comparator is a good idea.

@MichaelChirico MichaelChirico added this to the 3.1.2 milestone Oct 12, 2023
@MichaelChirico
Copy link
Collaborator Author

expect_true(info = "my info first", a > b)

this doesn't lint, it's unusual enough that I would wait for a request first before supporting that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants