Skip to content

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

Merged
IndrajeetPatil merged 3 commits intomainfrom
length_levels
Aug 8, 2023
Merged

length_levels_linter() for length(levels(x)) --> nlevels(x)#2051
IndrajeetPatil merged 3 commits intomainfrom
length_levels

Conversation

@MichaelChirico
Copy link
Copy Markdown
Collaborator

Part of #884

@codecov-commenter
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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