New unnecessary_nesting_linter

[MichaelChirico]

Part of #884

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (05baf80) 99.40% compared to head (e63967b) 99.40%.

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2302   +/-   ##
=======================================
  Coverage   99.40%   99.40%           
=======================================
  Files         122      123    +1     
  Lines        5523     5584   +61     
=======================================
+ Hits         5490     5551   +61     
  Misses         33       33           

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

[AshesITR]

LOG should not be default, and probably we should have an arg for configuration.

[MichaelChirico]

yep... left it in here as a reminder to discuss what sort of parameterization we should build in here, WDYT?

allow_signal_calls = c("warning", "message", "cat", "stopifnot")?

[AshesITR]

I'm not 100% sure yet.

[AshesITR]

So, just to understand this: These functions explicitly allow a parallel branch in if statements with a single control-flow breaking statement?

[MichaelChirico]

Right... they're allowed to pair with "exit" clauses, but can't cause a lint by themselves -- nested code using them can't always be unnested. IOW they can remove a lint, but not cause a lint.

[MichaelChirico]

won't lint -- only the terminal call is examined

[AshesITR]

So

if (A) {
  stop()
} else {
  warning()
  do()
}

Would lint?

[MichaelChirico]

yes

[MichaelChirico]

Of course, one could always disable this behavior (and always force unnesting if possibe) by passing e.g. character() to the argument accepting a vector of calls.

[AshesITR]

I'm just wondering how intuitive the exception logic is.

[AshesITR]

I'm not 100% sure yet.

[AshesITR]

I think the path forward is to carve out an exception when the only expression inside { is an assignment. Shall we mark that with a parameter to unnecessary_nesting_linter()?

SGTM. Should default to not linting (so all_linters() doesn't create a conflicting configuration).
Argument allow_assignment = TRUE?

[AshesITR]

I think the exceptions need some additional thought. Two options:

1. Remove exceptions for now and make a follow up issue.
2. Flesh out the interface and what should be eligible for exception.

Which do you prefer?

[MichaelChirico]

I think the exceptions need some additional thought. Two options:

1. Remove exceptions for now and make a follow up issue.
2. Flesh out the interface and what should be eligible for exception.

Which do you prefer?

let's go with one, I'm for making these PRs as sleek as possible in general:

#2325

[AshesITR]

Parameterize the list of allowed functions?

[MichaelChirico]

Have something specific in mind? Per the comment, switch() is included here as a control flow which happens to be a function, so it's treated the same as if/for/while/repeat.

[AshesITR]

Those discussed in #2326 come to mind.

[MichaelChirico]

Let's follow up in #2334, removed the exception for switch() for now.