-
Notifications
You must be signed in to change notification settings - Fork 193
Handle comments in unreachable_code_linter #2900
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
base: main
Are you sure you want to change the base?
Conversation
R/unreachable_code_linter.R
Outdated
#' @seealso [linters] for a complete list of linters available in lintr. | ||
#' @export | ||
unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclude_end", "# nocov end")) { | ||
# nolint next: object_usage_linter. Used in glue() in statically-difficult fashion to detect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can both remove the lint and break up the code into more digestible parts by only glueing once like
glue::glue("
({expr_after_control} | {last_function_call})/{unreachable_expr_ws} |
({expr_after_control} | {last_function_call})/{unreachable_expr_sc} |
({expr_after_control})/{next_or_break}/{unreachable_expr_ws} |
({expr_after_control})/{next_or_break}/{unreachable_expr_sc}
")
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did what you had in mind -- merge the logic for the [return/stop] and [next/break] cases, then use lint customization to give the right message in every case. That includes improving the customization beyond what existed already to mention return()
/ stop()
specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking back :)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2900 +/- ##
=======================================
Coverage ? 99.27%
=======================================
Files ? 128
Lines ? 7211
Branches ? 0
=======================================
Hits ? 7159
Misses ? 52
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* Robust linters for comments in "natural" places * revert whats in #2900 already * missed file not matching linter names
Split off from #2899, this one is large enough to get its own PR. Part of #2827. Also progress on #2737.