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_lintr adds .lintr to .Rbuildignore #2396

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@
* `unnecessary_lambda_linter` is extended to encourage vectorized comparisons where possible, e.g. `sapply(x, sum) > 0` instead of `sapply(x, function(x) sum(x) > 0)` (part of #884, @MichaelChirico). Toggle this behavior with argument `allow_comparison`.
* `backport_linter()` is slightly faster by moving expensive computations outside the linting function (#2339, #2348, @AshesITR and @MichaelChirico).
* `Linter()` has a new argument `linter_level` (default `NA`). This is used by `lint()` to more efficiently check for expression levels than the idiom `if (!is_lint_level(...)) { return(list()) }` (#2351, @AshesITR).
* `use_lintr()` adds the created `.lintr` file to the `.Rbuildignore` if run in a package (#1805, @MEO265).
* `string_boundary_linter()` recognizes regular expression calls like `grepl("^abc$", x)` that can be replaced by using `==` instead (#1613, @MichaelChirico).
* `unreachable_code_linter()` has an argument `allow_comment_regex` for customizing which "terminal" comments to exclude (#2327, @MichaelChirico). `# nolint end` comments are always excluded, as are {covr} exclusions (e.g. `# nocov end`) by default.
* `format()` and `print()` methods for `lint` and `lints` classes get a new option `width` to control the printing width of lint messages (#1884, @MichaelChirico). The default is controlled by a new option `lintr.format_width`; if unset, no wrapping occurs (matching earlier behavior).


Copy link
Collaborator

Choose a reason for hiding this comment

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

Superfluous change

### New linters

* `condition_call_linter()` for ensuring consistent use of `call.` in `warning()` and `stop()`. The default `call. = FALSE` follows the tidyverse guidance of not displaying the call (#2226, @Bisaloo)
Expand Down
36 changes: 34 additions & 2 deletions R/use_lintr.R
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#' Use lintr in your project
#'
#' Create a minimal lintr config file as a starting point for customization
#' Create a minimal lintr config file as a starting point for customization and add it to the .Rbuildignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be add it to .Rbuildignore, without a "the". Same in other places.

#'
#' @param path Path to project root, where a `.lintr` file should be created.
#' If the `.lintr` file already exists, an error will be thrown.
Expand All @@ -25,7 +25,7 @@
#' lintr::lint_dir()
#' }
use_lintr <- function(path = ".", type = c("tidyverse", "full")) {
config_file <- normalizePath(file.path(path, lintr_option("linter_file")), mustWork = FALSE)
config_file <- normalizePath(file.path(path, lintr_option("linter_file")), mustWork = FALSE, winslash = "/")
AshesITR marked this conversation as resolved.
Show resolved Hide resolved
if (file.exists(config_file)) {
stop("Found an existing configuration file at '", config_file, "'.", call. = FALSE)
}
Expand All @@ -43,5 +43,37 @@ use_lintr <- function(path = ".", type = c("tidyverse", "full")) {
)
)
write.dcf(the_config, config_file, width = Inf)

if (file.exists(file.path(path, "DESCRIPTION"))) {
# Some OS can only normalize a path if the associated file or folder exists, so the path needs to be re-normalized
tryCatch({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this part.
What do these systems return with normalizePath("foo", mustWork = FALSE) if foo doesn't exist?

pkg_path <- normalizePath(path, mustWork = TRUE, winslash = "/")
config_file <- normalizePath(file.path(path, lintr_option("linter_file")), mustWork = TRUE, winslash = "/")
}, error = function(e) {
stop("No entry could be added to the .Rbuildignore.", call. = FALSE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sounds like a warning is in order. The main side effect was caused at this point anyway.

})
# Check if config_file is in package i.e. lintr_option("linter_file") != "../.lintr"
if (startsWith(config_file, prefix = pkg_path)) {
# Skip a extra character for the leading `/`
rel_path <- substring(config_file, first = nchar(pkg_path) + 2L, last = nchar(config_file))
ignore_path <- file.path(pkg_path, ".Rbuildignore")
if (!file.exists(ignore_path)) file.create(ignore_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This branch can be a writeLines(..., ignore_path) with immediate return.

# Follow the same procedure as base R to see if the file is already ignored
ignore <- tryCatch({
trimws(readLines(ignore_path))
}, warning = function(e) {
cat(file = ignore_path, "\n", append = TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you silently fixing the missing terminal newline here?

trimws(readLines(ignore_path))
})
ignore <- ignore[nzchar(ignore)]
already_ignored <-
any(vapply(ignore, FUN = grepl, x = rel_path, perl = TRUE, ignore.case = TRUE, FUN.VALUE = logical(1L)))
if (!already_ignored) {
cat(file = ignore_path, rex::rex(start, rel_path, end), sep = "\n", append = TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IINM the sep= is ignored because you only pass one entry into cat.

message("Adding ", rel_path, " to .Rbuildignore")
}
}
}

invisible(config_file)
}
2 changes: 1 addition & 1 deletion man/use_lintr.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 53 additions & 0 deletions tests/testthat/test-use_lintr.R
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,56 @@ test_that("use_lintr with type = full also works", {
lints <- lint_dir(tmp)
expect_length(lints, 0L)
})

test_that("No .Rbuildignore is created of packages", {
tmp <- withr::local_tempdir()

lintr_file <- use_lintr(path = tmp, type = "full")
expect_false(file.exists(file.path(tmp, ".Rbuildignore")))
})

test_that("No .Rbuildignore is filled outside of packages", {
tmp <- withr::local_tempdir()
ignore <- file.path(tmp, ".Rbuildignore")
file.create(ignore)

lintr_file <- use_lintr(path = tmp, type = "full")
expect_identical(readLines(ignore), character())
})

test_that("No .Rbuildignore is filled if matching regex exists", {
tmp <- withr::local_tempdir()
file.create(file.path(tmp, "DESCRIPTION"))
ignore <- file.path(tmp, ".Rbuildignore")
file.create(ignore)
cat(file = ignore, ".*", sep = "\n")

lintr_file <- use_lintr(path = tmp, type = "full")
expect_identical(readLines(ignore), ".*")
})

test_that("use_lintr creates the correct regex", {
tmp <- withr::local_tempdir()
file.create(file.path(tmp, "DESCRIPTION"))
ignore <- file.path(tmp, ".Rbuildignore")
file.create(ignore)
cat(file = ignore, "^fu$", "^bar$", sep = "\n")

expect_message({
lintr_file <- use_lintr(path = tmp, type = "full")
}, regexp = "Adding .* to .Rbuildignore")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: the . in front of Rbuildignore matches any character.

expect_identical(readLines(ignore), c("^fu$", "^bar$", "^\\.lintr$"))
})

test_that("use_lintr handles missing final new line", {
tmp <- withr::local_tempdir()
file.create(file.path(tmp, "DESCRIPTION"))
ignore <- file.path(tmp, ".Rbuildignore")
file.create(ignore)
cat(file = ignore, "^fu$\n^bar$")

expect_message({
lintr_file <- use_lintr(path = tmp, type = "full")
}, regexp = "Adding .* to .Rbuildignore")
expect_identical(readLines(ignore), c("^fu$", "^bar$", "^\\.lintr$"))
})
Loading