Adds expect_that() to undesirable functions linter

[IndrajeetPatil]

If code is expected to produce an error, using expect_that(x, throws_error()) works but is unadvised. It is better to use the function dedicated specifically for checking errors: expect_error(x). The additional benefit is that you can also check for the expected error message using regular expressions.

First time making a PR to lintr, so any help in getting the PR aligned with coding standards (if I'm violating any) adopted in this repo is greatly appreciated.

[AshesITR]

Thanks for taking the time to create a PR - it looks quite good already.
From a meta point of view: Is there any circumstance where expect_that() is preferrable to other expectations?

It might also be worthwhile to call this linter expect_that_linter() and have it lint all flavors of expect_that(x, some_condition)where there is an appropriate equivalent expect_some_condition(x). WDYT?

[MichaelChirico]

It might also be worthwhile to call this linter expect_that_linter() and have it lint all flavors of expect_that(x, some_condition)where there is an appropriate equivalent expect_some_condition(x). WDYT?

Came here to say just that -- the documentation explicitly says:

An old style of testing that's no longer encouraged.

So linting this to encourage folks to move off of it (maybe with # nolint if there's something that can only be done with test_that? I am unfamiliar with this approach so I don't know) would be appropriate IMO

[MichaelChirico]

But then, isn't this just a use case for undesirable_function_linter? 🤔

[IndrajeetPatil]

Thanks for the feedback, both.

Yeah, I initially thought of writing expect_that_linter, but wasn't sure if it was too aggressive since expect_that() was superseded only recently.

But, given that we are just issuing a warning, I think it might be worth it.

But then, isn't this just a use case for undesirable_function_linter?

Wasn't aware of this linter. Yeah, I would agree then that this should just be part of this linter.

I will update the PR accordingly.

[IndrajeetPatil]

@MichaelChirico, @AshesITR WDYT? Is this better?

[MichaelChirico]

Again per the docs:

https://github.com/r-lib/testthat/blob/426eb523f6772c76175d6e8d7ca04355b1ee7ad5/R/expect-that.R#L8-L9

So actually this function is deprecated in 3e... that eliminates any hesitation I had about adding it as a default undesirable. But let's also make note of that in the lint message/documentation.

[IndrajeetPatil]

But let's also make note of that in the lint message/documentation

You mean we should mention that this function is deprecated in 3e in the message/documentation?

[MichaelChirico]

You mean we should mention that this function is deprecated in 3e in the message/documentation?

yep, exactly

[IndrajeetPatil]

@MichaelChirico I've updated the message. Lemme know if you want me to change the wording.

[MichaelChirico]

be sure to add a NEWS bullet & cite yourself!

[IndrajeetPatil]

Done!

[MichaelChirico]

LGTM, but now I'm wondering -- why stop here? shouldn't we add all deprecated tidyverse functions here? 🤔

WDYT @AshesITR should we just merge this now and consider that in follow-up?

[IndrajeetPatil]

Hmm, I would say that that would be expanding the scope of this PR beyond its initial purview.

It'd be better to create a new issue solely for tidyverse's deprecated functions, and then I can make a separate PR targeting that particular issue.

Sounds reasonable?

[AshesITR]

I agree. Let's create a follow-up issue (maybe one per package?) to keep the PR focused.

Are there other deprecated functions in testthat?

[IndrajeetPatil]

expect_known_output(),
expect_known_value(),
expect_known_hash(),
expect_equal_to_reference()
expect_equivalent()
expect_reference()
expect_is()
setup()
teardown()

I am not sure about linting these, though.

If you are using 3rd edition of testthat, testthat itself will warn you about these functions being deprecated in the 3rd edition.

[MichaelChirico]

there are a bunch:

expect_more_than(), expect_less_than()
context()
expect_equivalent(), expect_reference()
with_mock(), local_mock()
expect_is()

everything in these files:

https://github.com/r-lib/testthat/blob/f8ccf033a74d89e2d1d3cb2ec3c6c4f63ca69055/R/old-school.R

https://github.com/r-lib/testthat/blob/f8ccf033a74d89e2d1d3cb2ec3c6c4f63ca69055/R/deprec-condition.R

https://github.com/r-lib/testthat/blob/6666662844274e8fa1988c8e0cfecf0b13399ee1/R/expect-known.R

---

an added complication is some are plain deprecated, others are 3e-deprecated only. maybe what we want to do is export an object to encapsulate all this.

because that would entail a user-facing change vs this PR as of now, I lean towards tabling this until after 3.0.0 release so we can handle the design more carefully, WDYT?

[IndrajeetPatil]

I lean towards tabling this until after 3.0.0 release so we can handle the design more carefully, WDYT?

Sounds good. I am marking this as a draft for now. We can come back to it later.

Similar to undesirable_function_linter(), I am wondering if we should introduce a new deprecated_function_linter() in the next release. Functions like expect_that() are more suitable for this linter than undesirable linter.

[IndrajeetPatil]

Closing in favour of #1386.

[MichaelChirico]

Thanks again for the PR! Sorry it didn't land, & thank you for your understanding. Looking forward to the bigger & better version!