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

length_levels_linter() for length(levels(x)) --> nlevels(x) #2051

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

MichaelChirico
Copy link
Collaborator

Part of #884

@codecov-commenter
Copy link

Codecov Report

Merging #2051 (86ea91a) into main (6fc5570) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 86ea91a differs from pull request most recent head 7d075f8. Consider uploading reports for the commit 7d075f8 to get more accurate results

@@           Coverage Diff           @@
##             main    #2051   +/-   ##
=======================================
  Coverage   98.56%   98.56%           
=======================================
  Files         114      115    +1     
  Lines        5017     5032   +15     
=======================================
+ Hits         4945     4960   +15     
  Misses         72       72           
Files Changed Coverage Δ
R/length_levels_linter.R 100.00% <100.00%> (ø)

@AshesITR
Copy link
Collaborator

AshesITR commented Aug 8, 2023

There are also data.table::uniqueN() and dplyr::n_distinct() for the similar length(unique(x)). WDYT about optionally linting this here?

@MichaelChirico
Copy link
Collaborator Author

I would put that as a separate linter.

The closer analogue is nested_ifelse_linter(), whose primary recommended alternatives are data.table::fcase() and dplyr::case_when().

But in that case, there are ways to avoid the nested ifelse() using base functions, so the user doesn't strictly need any new packages to fix the issue.

For length(unique(.)), there's no improvement in {base} (AFAIK).

Doesn't mean we shouldn't offer such a linter (since it'll be optional for users to turn on anyway), just by way of explaining why we didn't write that so far (it's certainly crossed my mind).

@IndrajeetPatil IndrajeetPatil merged commit 3f22b76 into main Aug 8, 2023
@IndrajeetPatil IndrajeetPatil deleted the length_levels branch August 8, 2023 05:20
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.

4 participants