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

New stopifnot_all_linter #2273

Merged
merged 6 commits into from
Nov 13, 2023
Merged

New stopifnot_all_linter #2273

merged 6 commits into from
Nov 13, 2023

Conversation

MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented Nov 10, 2023

Part of #884

No lints on our own package

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

Merging #2273 (53f6fd7) into main (71c7e31) will not change coverage.
The diff coverage is n/a.

❗ Current head 53f6fd7 differs from pull request most recent head 530bc92. Consider uploading reports for the commit 530bc92 to get more accurate results

@@           Coverage Diff           @@
##             main    #2273   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files         114      114           
  Lines        5258     5258           
=======================================
  Hits         5239     5239           
  Misses         19       19           

@AshesITR
Copy link
Collaborator

WDYT about integrating it into conjunct_test_linter()? It detects a similar problem with stopifnot().

@MichaelChirico
Copy link
Collaborator Author

WDYT about integrating it into conjunct_test_linter()? It detects a similar problem with stopifnot().

I lean towards "no", since it's really a particular thing for stopifnot() (e.g. testthat::expect_true() nor assertthat::assert_that() use this behavior).

That said, we don't have to stretch the definition of "conjunct" too much to make it work -- all(x) being x[1L] && ... && x[n]...

@AshesITR
Copy link
Collaborator

Metadata and vectorization tests would be nice.

@MichaelChirico
Copy link
Collaborator Author

vectorization tests

I think for linters built with make_linter_from_xpath(), that's overkill -- better to test make_linter_from_xpath() directly

@AshesITR
Copy link
Collaborator

If the linter ever gets more complex functionality in the future, these tests will ensure that it still works?

@MichaelChirico
Copy link
Collaborator Author

If the linter ever gets more complex functionality in the future, these tests will ensure that it still works?

I think the next commit would be the one to owe new tests.

OTOH, it doesn't really hurt to add new tests. And more importantly, there is a new test of the XPath logic here, namely that it's not somehow over- or under-counting lints. Not sure we've come across that being an issue in the past, but still justifies the new test IMO.

@AshesITR AshesITR merged commit acee3bb into main Nov 13, 2023
20 checks passed
@AshesITR AshesITR deleted the stopifnot_all branch November 13, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants