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

consecutive_stopifnot_linter should include assert_that (and hence be renamed) #1604

Closed
MichaelChirico opened this issue Oct 3, 2022 · 2 comments · Fixed by #1940
Closed
Labels
breaking change ☠️ API change likely to affect existing code feature a feature request or enhancement

Comments

@MichaelChirico
Copy link
Collaborator

Similar reasoning for consecutive_stopifnot_linter() applies for using assert_that(); assert_that(); assert_that(); unless a message is specified, it's better to use the ... and condense to one assert_that() call.

This will entail a deprecation cycle for consecutive_stopifnot_linter(), so let's align on a name before continuing.

I think consecutive_assertion_linter() fits well, but differs from conjunct_test_linter() (which also covers assert_that() and stopifnot()).

consecutive_test_linter() sounds more like it would apply to testthat functions, so I'm not sold on it.

@MichaelChirico MichaelChirico added feature a feature request or enhancement breaking change ☠️ API change likely to affect existing code labels Oct 3, 2022
@IndrajeetPatil
Copy link
Collaborator

I'd also vote for consecutive_assertion_linter()!

@AshesITR
Copy link
Collaborator

AshesITR commented Oct 3, 2022

I'm not too worried about renaming since assert_that() is basically stopifnot(all(...)).

I remember some discussion wrt coonjunct_assertion_linter() as a potential name for conjunct_test_linter(), but can't remember why we settled on a different name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ☠️ API change likely to affect existing code feature a feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants