use_lintr adds .lintr to .Rbuildignore
#2396
Conversation
|
Here you can see how base R handles the .Rbuildignore: |
Surely. I'll take care of that too. Just wanted to hear beforehand whether additional functionality would be desired. In order not to have to adapt the tests again and again. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2396 +/- ##
==========================================
- Coverage 98.68% 98.65% -0.04%
==========================================
Files 126 126
Lines 5700 5721 +21
==========================================
+ Hits 5625 5644 +19
- Misses 75 77 +2 ☔ View full report in Codecov by Sentry. |
It runs locally on Windows, which surprises me
Some OS can only normalize a path if the associated file or folder exists
|
The code is a bit more complex than initially expected, but it should work now |
MEO265
changed the title
use_lintradds .lintr to .Rbuildignoreuse_lintr adds .lintr to .Rbuildignore
|
|
||
| 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({ |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
This sounds like a warning is in order. The main side effect was caused at this point anyway.
| # 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) |
There was a problem hiding this comment.
This branch can be a writeLines(..., ignore_path) with immediate return.
| ignore <- tryCatch({ | ||
| trimws(readLines(ignore_path)) | ||
| }, warning = function(e) { | ||
| cat(file = ignore_path, "\n", append = TRUE) |
There was a problem hiding this comment.
Why are you silently fixing the missing terminal newline here?
| 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) |
There was a problem hiding this comment.
IINM the sep= is ignored because you only pass one entry into cat.
|
|
||
| expect_message({ | ||
| lintr_file <- use_lintr(path = tmp, type = "full") | ||
| }, regexp = "Adding .* to .Rbuildignore") |
There was a problem hiding this comment.
Nit: the . in front of Rbuildignore matches any character.
| * `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). | ||
|
|
||
|
|
| @@ -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 | |||
There was a problem hiding this comment.
It should be add it to .Rbuildignore, without a "the". Same in other places.
Closes #1805
I know that a PR (#1895) already exists, but it doesn't seem to be moving forward anymore.
Please give me your opinion on whether information should be output or additional parameters should be added.