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.

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

Linter to recommend new expectations to check for absence of exceptions? #1874

Open
IndrajeetPatil opened this issue Dec 23, 2022 · 8 comments

Comments

@IndrajeetPatil
Copy link
Collaborator

{testthat} has recently introduced new expectations that assert the absence of an error, warning, or message.

These expectations are (IMO: r-lib/testthat#1642) far more readable than the previous idiom of setting regexp = NA.

Should we introduce a new linter that recommends using these new expectations?

# actual (forces reader to figure out what the second argument is and what setting it to NA entails)
expect_message({ ... }, NA)
expect_warning({ ... }, NA)
expect_error({ ... }, NA)
expect_condition({ ... }, NA)

# suggested
expect_no_message({ ... })
expect_no_warning({ ... })
expect_no_error({ ... })
expect_no_condition({ ... })

P.S. A fly in the ointment is that the lifecycle for these new expectations is experimental. So maybe we should wait until it's stable?

@AshesITR
Copy link
Collaborator

Fine with me. Probably should wait for the lifecycle to upgrade.

@IndrajeetPatil
Copy link
Collaborator Author

IndrajeetPatil commented May 27, 2024

In testthat 3.2.0 release, these functions were marked as stable, so we could work on this issue:

image

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented May 27, 2024

we can do expect_no_linter() to cover these and our expect_no_lint()?

that's very close in name to expect_not_linter(), my first instinct is not to combine them...

@AshesITR
Copy link
Collaborator

AshesITR commented May 27, 2024

I was thinking expect_condition_linter() or expect_signal_linter(), but neither of those incorporate expect_lint()...

NB our condition_call_linter() and condition_message_linter().

@AshesITR
Copy link
Collaborator

WDYT about expect_no_condition_linter()?

@MichaelChirico
Copy link
Collaborator

I think it's good enough. a lint is a "condition", right? 😄

@AshesITR
Copy link
Collaborator

AshesITR commented May 27, 2024

SGTM. I'm starting to grasp why Pylint uses numbers for lints 😂

@F-Noelle
Copy link
Contributor

expect_absence_linter, perhaps? given that this is how they seem to be described collectively.

Also expect_no_lint, expect_lint do have checks as the secondary parameter which is also equal to NULL and not NA.

Not saying that this linter shouldn't handle expect_no_lint with the other absence functions, but it is distinct separate logic that needs to be handled as well.

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

No branches or pull requests

4 participants