Skip to content
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

Use {cli} progress bar #2641

Merged
merged 12 commits into from
Dec 3, 2024
3 changes: 0 additions & 3 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,10 @@ importFrom(stats,na.omit)
importFrom(tools,R_user_dir)
importFrom(utils,capture.output)
importFrom(utils,getParseData)
importFrom(utils,getTxtProgressBar)
importFrom(utils,globalVariables)
importFrom(utils,head)
importFrom(utils,relist)
importFrom(utils,setTxtProgressBar)
importFrom(utils,tail)
importFrom(utils,txtProgressBar)
importFrom(xml2,as_list)
importFrom(xml2,xml_attr)
importFrom(xml2,xml_children)
Expand Down
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@

## Notes

* All user-facing messages (including progress bars) are now prepared using the `{cli}` package (#2418 and #2641, @IndrajeetPatil). All messages have been reviewed and updated to be more informative and consistent.
* All user-facing messages are now prepared using the `{cli}` package (#2418, @IndrajeetPatil). All messages have been reviewed and updated to be more informative and consistent.
* File locations in lints and error messages contain clickable hyperlinks to improve code navigation (#2645, #2588, @olivroy).
* {lintr} now depends on R version 4.0.0. It already does so implicitly due to recursive upstream dependencies requiring this version; we've simply made that dependency explicit and up-front (#2569, @MichaelChirico).
Expand Down
35 changes: 16 additions & 19 deletions R/lint.R
Original file line number Diff line number Diff line change
Expand Up @@ -182,20 +182,24 @@
return(lints)
}

pb <- if (isTRUE(show_progress)) {
txtProgressBar(max = length(files), style = 3L)
if (isTRUE(show_progress)) {
lints <- lapply(
# NB: This cli API is experimental (https://github.com/r-lib/cli/issues/709)
cli::cli_progress_along(files, name = "Running linters"),
function(idx) {
lint(files[idx], ..., parse_settings = FALSE, exclusions = exclusions)
}
)
} else {
lints <- lapply(
files,
function(file) {

Check warning on line 196 in R/lint.R

View workflow job for this annotation

GitHub Actions / lint

file=R/lint.R,line=196,col=7,[unnecessary_lambda_linter] Pass lint directly as a symbol to lapply() instead of wrapping it in an unnecessary anonymous function. For example, prefer lapply(DF, sum) to lapply(DF, function(x) sum(x)).
Copy link
Collaborator Author

@IndrajeetPatil IndrajeetPatil Dec 2, 2024

Choose a reason for hiding this comment

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

Should I just nolint this?

It's catch-22 situation:

  • if I use anonymous function, it triggers unnecessary_lambda_linter() lint
  • if I don't, it leads to Error: The use of linters of class 'function' was deprecated in lintr version 3.0.0 error

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's interesting! I don't understand why the error. Yes, mark for # nolint, but please tag it with a TODO+follow-up bug to dive deeper on that to see if there's some bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to nolint it.

lint(file, ..., parse_settings = FALSE, exclusions = exclusions)
}
)
}

lints <- flatten_lints(lapply(
files,
function(file) {
maybe_report_progress(pb)
lint(file, ..., parse_settings = FALSE, exclusions = exclusions)
}
))

if (!is.null(pb)) close(pb)

lints <- flatten_lints(lints)
lints <- reorder_lints(lints)

if (relative_path) {
Expand Down Expand Up @@ -688,13 +692,6 @@
!nzchar(names2(dots)[1L])
}

maybe_report_progress <- function(pb) {
if (is.null(pb)) {
return(invisible())
}
setTxtProgressBar(pb, getTxtProgressBar(pb) + 1L)
}

maybe_append_error_lint <- function(lints, error, lint_cache, filename) {
if (is_lint(error)) {
error$linter <- "error"
Expand Down
3 changes: 1 addition & 2 deletions R/lintr-package.R
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@
#' @importFrom rex rex regex re_matches re_substitutes character_class
#' @importFrom stats na.omit
#' @importFrom tools R_user_dir
#' @importFrom utils capture.output getParseData getTxtProgressBar globalVariables head relist
#' setTxtProgressBar tail txtProgressBar
#' @importFrom utils capture.output getParseData globalVariables head relist tail
#' @importFrom xml2 as_list
#' xml_attr xml_children xml_find_all xml_find_chr xml_find_lgl xml_find_num xml_find_first xml_name xml_text
## lintr namespace: end
Expand Down
6 changes: 0 additions & 6 deletions tests/testthat/test-lint_dir.R
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,6 @@ test_that("lint all files in a directory", {

expect_s3_class(lints, "lints")
expect_identical(sort(linted_files), sort(files))

expect_output(
lint_dir(the_dir, parse_settings = FALSE, show_progress = TRUE),
"======",
fixed = TRUE
)
})

test_that("lint all relevant directories in a package", {
Expand Down
Loading