New parameters for if_switch_linter() to optionally skip lints on "complex" if/else usage

[MichaelChirico]

Closes #2322

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e0a1d24) 98.55% compared to head (e7e1227) 98.55%.

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2413   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files         126      126           
  Lines        5735     5762   +27     
=======================================
+ Hits         5652     5679   +27     
  Misses         83       83           

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

[AshesITR]

Should this lint switch() calls that violate the max_* bounds, too?

[MichaelChirico]

Should this lint switch() calls that violate the max_* bounds, too?

Interesting... possibly? I would wait for user request though, WDYT?

[AshesITR]

I feel the linter should enforce some consistency.
The reason why the lints would be suppressed are also valid reasons why the switch() should be broken up into an if () ... else if () ... chain.

[MichaelChirico]

OK, let me double-check how this should work as I'm second-guessing myself trying to wrap my head around it.

# always lint if/else if/else if usage
# never lint switch()
if_switch_linter(max_branch_lines = 0, max_branch_expr = 0)

# only lint if/else if/else if of "low" complexity
# also lint switch() of "high" complexity
if_switch_linter(max_branch_lines = 2, max_branch_expr = 3)

i.e. these parameters require using if/else if/.../else for high-complexity switch-y cases, and require switch() for low-complexity switch-y cases. Is that right?

[AshesITR]

Looks good, I would make some of the tests "unbalanced" in terms of lines/exprs per branch to ensure the largest/most complex branch dictates behavior.

AFAICT there is no such test for switch().
Also, is there a metadata test for switch()?

[MichaelChirico]

AFAICT there is no such test for switch().

IIUC those are https://github.com/r-lib/lintr/pull/2413/files#diff-5a5a1de0b476f8f7c4ffcfbf4f4307c8cdfbbfa85367eccf61060e48e433c9e5R337-R340 and https://github.com/r-lib/lintr/pull/2413/files#diff-5a5a1de0b476f8f7c4ffcfbf4f4307c8cdfbbfa85367eccf61060e48e433c9e5R361-R364

Also, is there a metadata test for switch()?

No, adding now

[AshesITR]

AFAICT there is no such test for switch().

IIUC those are https://github.com/r-lib/lintr/pull/2413/files#diff-5a5a1de0b476f8f7c4ffcfbf4f4307c8cdfbbfa85367eccf61060e48e433c9e5R337-R340 and https://github.com/r-lib/lintr/pull/2413/files#diff-5a5a1de0b476f8f7c4ffcfbf4f4307c8cdfbbfa85367eccf61060e48e433c9e5R361-R364

In those tests, all switch branches have the same complexity (loc & expr count). That's what I meant by "balanced".

I'd add something like

switch(x,
  a = 1
  b = {
    2
    3
    4; 5; 6
  },
  c = {
    7
  }
)

to the mix

Also, is there a metadata test for switch()?

No, adding now

[MichaelChirico]

Thanks for clarifying, added some tests unbalanced across branches