Unreachable loops

[MEO265]

Closes #2105

It would also be possible to put the two cases stop/return and next/break together. Then there would be no separate message and more than the sensible situations would be tested, e.g. next in functions, but it would be faster.

I would be grateful for a hint as to why it is only necessary to filter out the end of "nolint" blocks but not the beginning

[MEO265]

I realized I forgot for loops, after that it becomes a normal PR

[MichaelChirico]

I would be grateful for a hint as to why it is only necessary to filter out the end of "nolint" blocks but not the beginning

See the original #1347. The idea is that we don't want to lint as "unreachable comment" some code that only exists to terminate a # nolint region.

In principle a # nolint start region could begin there, too, but in practice I've never seen this. Plain # nolint are also ignored since they should only come after actual expressions -- the expression itself will get the unreachable_codee_linter() hit, so no need to double that up by linting the comment as well.

[codecov-commenter]

Codecov Report

Merging #2141 (e30e1a7) into main (6e4dbec) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main    #2141   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         114      114           
  Lines        5176     5198   +22     
=======================================
+ Hits         5158     5180   +22     
  Misses         18       18           

Files	Coverage Δ
R/unreachable_code_linter.R	100.00% <100.00%> (ø)

[MichaelChirico]

long tests like this are good for robustness, but best to start with very succinct tests of only one aspect of the lint logic.

it means more lines of code which can be tedious, but is much preferable for test readability & ease of maintenance

[MichaelChirico]

it may also help to either annotate the test with a comment on what all is being tested (it looks like "various types of if/else cases"), or to split into different test_that() and record that in the test's name

[MichaelChirico]

Hi @MEO265, we're closing in on sealing up a release (lintr 3.1.1). If you'd like this PR included, please take a chance to wrap up. If you'd like it included and won't have time to work on it in the next few weeks, let us know and we can try taking over from here. Thanks!

[MEO265]

Hi @MEO265, we're closing in on sealing up a release (lintr 3.1.1). If you'd like this PR included, please take a chance to wrap up. If you'd like it included and won't have time to work on it in the next few weeks, let us know and we can try taking over from here. Thanks!

Unfortunately, I have absolutely no idea whether I will find time this week or next. I would fix everything the following Monday at the latest.
But feel free to change things yourself, especially if it doesn't fit with the release otherwise.

[MichaelChirico]

Hi @MEO265, friendly ping :)

[MichaelChirico]

Fixed the more trivial requests. I'll let the long tests slide for now, @MEO265 a follow-up improving the tests would be appreciated if you find the time! Thanks for the PR.