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.

Sign up for GitHub

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

Jump to bottom

Print a message when no lints found #2643

Merged
merged 8 commits into from
Aug 5, 2024
Merged

Print a message when no lints found #2643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f4ceedb
Print a message when no lints found
IndrajeetPatil Aug 4, 2024
223b98e
use info markup
IndrajeetPatil Aug 4, 2024
24f461b
Update NEWS.md
IndrajeetPatil Aug 4, 2024
78939d2
delint
IndrajeetPatil Aug 4, 2024
b0a2423
more inlining
IndrajeetPatil Aug 4, 2024
63b0017
use single if-else
IndrajeetPatil Aug 4, 2024
de94d8f
Create a separate NEWS entry
IndrajeetPatil Aug 5, 2024
b8dfcca
don't install quarto to avoid NOTE
IndrajeetPatil Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/R-CMD-check-hard.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ jobs:

- uses: r-lib/actions/setup-r-dependencies@v2
with:
install-quarto: false
pak-version: devel
dependencies: '"hard"'
cache: false
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/R-CMD-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ jobs:

- uses: r-lib/actions/setup-r-dependencies@v2
with:
install-quarto: false
extra-packages: |
any::rcmdcheck
needs: check
Expand Down
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
* `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico).
* `paste_linter()` is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico).
* `expect_no_lint()` was added as new function to cover the typical use case of expecting no lint message, akin to the recent {testthat} functions like `expect_no_warning()` (#2580, @F-Noelle).
* `lint()` and friends emit a message if no lints are found (#2643, @IndrajeetPatil).

### New linters

Expand Down
10 changes: 7 additions & 3 deletions R/methods.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,14 @@ print.lints <- function(x, ...) {
if (isTRUE(settings$error_on_lint)) {
quit("no", 31L, FALSE) # nocov
}
} else if (use_rstudio_source_markers) {
# Empty lints: clear RStudio source markers
rstudio_source_markers(x)
} else {
# Empty lints
cli_inform(c(i = "No lints found."))
Copy link
Collaborator

@MichaelChirico MichaelChirico Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is a classed signal:

class(tryCatch(cli::cli_inform("xxx"), condition=identity))
# [1] "rlang_message" "message"       "condition"

Is that appropriate? I might have expected just cat() here, I guess {cli} adds nice aesthetics -- are there any analogues for "plain text to terminal"? cli::cli_text(c(i = "No lints found.")) drops the i formatting.

(or perhaps a message is appropriate, WDYT?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Message seems ok to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we could use cat(), but I also think there are benefits to having this as a classed signal over cat() (e.g. the messages will show up in logs).

If we don't want a classed signal, either cli_alert() or its sibling should work:

cli::cli_alert_success("No lints found")
#> ✔ No lints found

class(tryCatch(cli::cli_alert_success("No lints found"), condition=identity))
#> [1] "cli_message" "condition"

Created on 2024-08-05 with reprex v2.1.1

So we can have our pretty aesthetics cake and eat it too.

if (use_rstudio_source_markers) {
rstudio_source_markers(x) # clear RStudio source markers
}
}

invisible(x)
}

Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/test-methods.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ test_that("print.lint works", {
expect_output(print(l), " 1:length(x)", fixed = TRUE)
})

test_that("print.lint works with empty lints", {
withr::local_options(list(lintr.rstudio_source_markers = FALSE))
l <- lint(text = "1L")

expect_message(print(l), "No lints found", fixed = TRUE)
})

test_that("print.lint works for inline data, even in RStudio", {
l <- lint("x = 1\n")

Expand Down
Loading