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

Also lint nrow(filter(.)) in nrow_subset_linter() #2457

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Conversation

MichaelChirico
Copy link
Collaborator

Closes #2313

This is the simplest version. I was weighing a more complicated version a la conjunct_test_linter()'s allow_filter= argument where we also check that this is not stats::filter() and decided against it for now. FWIW, there are no hits for nrow(stats::filter( on GitHub (which of course doesn't preclude this being run without namespace-qualifying stats):

https://github.com/search?q=lang%3AR+%2Fnrow%5B%28%5D%5Cs*stats%3A%3Afilter%5B%28%5D%2F&type=code

I also left out new okay examples because AFAIK the with() approach is the simplest in {dplyr} world as well. The more {dplyr}-esque approach I can think of would be x |> summarize(n = sum(<filter>, na.rm = TRUE)) |> pull(n), but that looks way worse to me than with() (OTOH, it will work with all backends).

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7cb5023) 98.54% compared to head (a8ca6fb) 98.54%.

❗ Current head a8ca6fb differs from pull request most recent head b33346d. Consider uploading reports for the commit b33346d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2457   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         126      126           
  Lines        5720     5720           
=======================================
  Hits         5637     5637           
  Misses         83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelChirico MichaelChirico merged commit ccdb186 into main Dec 20, 2023
21 checks passed
@MichaelChirico MichaelChirico deleted the nrow-filter branch December 20, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nrow_subset_linter() should recognize filter() usage as well
3 participants