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

expect_is_linter, or just use undesirable_function_linter? #946

Closed
MichaelChirico opened this issue Mar 15, 2022 · 4 comments
Closed

expect_is_linter, or just use undesirable_function_linter? #946

MichaelChirico opened this issue Mar 15, 2022 · 4 comments

Comments

@MichaelChirico
Copy link
Collaborator

As part of #884, we wrote an ExpectIsLinter() to point out that testthat::expect_is() is deprecated.

The value-add vs. just using (rough syntax) undesirable_function_linter(expect_is = "Use expect_type or expect_s3_class") is that we run a similar check to expect_type_linter() to see if expect_type() or expect_s3_class() is more appropriate, and recommend accordingly, i.e., the customized linter is more customized for recommended action.

Filing this issue to poll whether this is worthwhile (the linter is already written and in use here, but there is some toil to converting, reviewing, and maintaining it)

@AshesITR
Copy link
Collaborator

Doesn't testthat report the deprecation?
Would be a good addition to default_undesirable_functions, though.

@MichaelChirico
Copy link
Collaborator Author

It's superseded in 2e, deprecated in 3e, so it's a bit in limbo:

https://testthat.r-lib.org/reference/expect_is.html

In general there are cases when it can be nice to surface issues without running any code (the namespace hook linters are similar -- eventually R CMD check will give a warning, but if lintr is fully integrated with your IDE that warning can come much sooner)

@AshesITR
Copy link
Collaborator

The downside is, this linter will become obsolete whenever testthat decides to remove the function.
I'd vote for updating default_undesirable_functions instead.

@MichaelChirico
Copy link
Collaborator Author

indeed. ok SGTM. much easier anyway 😅

the downside to us adding it is it opens the door to throwing deprecation warnings for any number of packages.

so actually I would lean towards lintr not doing anything.

if you think it's still worth pursuing, please open a separate issue about the more general topic of lintr's role in nudging users off deprecated functions. maybe we can think of an API to decentralize this, or come up with rules about what packages would be in scope.

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

2 participants