Skip to content

unreachable_code_linter() lints on code in if (FALSE), while (FALSE) #2123

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

Merged
merged 32 commits into from
Sep 9, 2023

Conversation

MEO265
Copy link
Contributor

@MEO265 MEO265 commented Sep 8, 2023

Resolves #1428.

I held back from linting comments with the new features since, unlike the previous linting scenario, I don't see any issues with it and didn't find any guidelines on this in the tidyverse style guide. But I'm open to tweaking this if needed. Maybe we could even add a flag for this option?

Empty loops are intentionally ignored.

When I get a moment, I'm also planning to tackle issue #2105.

I noticed that the previous examples each contained an unnecessary line in my opinion, but I didn't want to just change it. Am I missing something why writeLines(code_lines) is necessary?

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Merging #2123 (17fc0bf) into main (739b4b6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main    #2123   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         121      121           
  Lines        5232     5258   +26     
=======================================
+ Hits         5214     5240   +26     
  Misses         18       18           
Files Changed Coverage Δ
R/unreachable_code_linter.R 100.00% <100.00%> (ø)

@MEO265
Copy link
Contributor Author

MEO265 commented Sep 8, 2023

I have seen the lints; I will correct them.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work. Don't forget to add a NEWS bullet.

@MichaelChirico
Copy link
Collaborator

I held back from linting comments

Makes sense to me... even though some such comments should likely be removed, some are OK, and we won't have a way to distinguish them that I can think of.

@MichaelChirico
Copy link
Collaborator

I held back from linting comments

OTOH, what about cases like this?

if (FALSE) {
  # some_code()
  # that_i_commented_out()
  # that_should_be_removed()
}

@MichaelChirico
Copy link
Collaborator

Overall, looks good! Thanks for taking this on!

@MichaelChirico MichaelChirico changed the title Enhance unreachable code linter unreachable_code_linter() lints on code in if (FALSE), while (FALSE) Sep 9, 2023
@MEO265
Copy link
Contributor Author

MEO265 commented Sep 9, 2023

I held back from linting comments

OTOH, what about cases like this?

if (FALSE) {
  # some_code()
  # that_i_commented_out()
  # that_should_be_removed()
}

I think linter should have independent separate tasks and that would be the commented_code_linter's job

From
(1) run all xml_find_all()
(2) run all xml_nodes_to_lints()
to
(1) Run xml_find_all() and xml_nodes_to_lints() for XPath 1
(2) ...
(n) Run xml_find_all() and xml_nodes_to_lints() for XPath n
…nreachable_code_linter

# Conflicts:
#	R/unreachable_code_linter.R
@MichaelChirico MichaelChirico merged commit 78417b0 into r-lib:main Sep 9, 2023
@MEO265 MEO265 deleted the enhance_unreachable_code_linter branch September 10, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint if (FALSE) / while (FALSE) snippets through commented_code_linter?
4 participants