Support excluding entire directories #539

AshesITR · 2020-10-21T19:18:12Z

fixes #518
fixes #438

implement
test
document

The current solution is a bit inefficient since you could technically avoid listing the ignored directory.
Instead, we add full file exclusions for all files in the directories.

AshesITR · 2020-10-23T15:29:56Z

also needed to fix #438 for the .lintr config file based test case to succeed.

AshesITR · 2020-10-23T15:40:04Z

cc @russHyde @jimhester My hacky fix to #438 isn't quite as nice as #439 (currently not merged)
Do you have a preference regarding the approach to relative exclusions?

The approach in this PR is to temporarily setwd() in the only place where a directory prefix is necessary and set it back on.exit, namely when reading settings. #439 instead introduces a new argument to normalize_exclusions() to carry the path information.

We could also merge #439 first but its diff is kind of large at a quick glance.

russHyde · 2020-10-25T14:22:39Z

Hi @AshesITR , I've been quiet on the lintr front for 6 months (job & family reasons). I never actually finished #439 ; my changeset was pretty large to begin with, and a collaborator pushed some additional changes onto my fork of lintr; so the changeset got really large. I'd like to split it into smaller chunks so that it's more easy to review, but I'm not sure where to start.

I'll try to look over your changes in the coming week (haven't looked at them yet), but would urge against using setwd in lintr (& any CRAN-targetted code). Thanks for looking into directory exclusions, to be able to drop directories from the config would be really valuable.

AshesITR · 2020-10-26T20:46:03Z

A rebase to master caused some unexpected diffs...

Thanks @russHyde for the feedback so far - I've removed the call to setwd() instead conditionally adding root to the file path if it's not already an absolute path.

Edit: I've created a squashed rebase commit f29437a.
That contains all the changes specific to this PR

fixes #438 as well as a few unrelated test failures on non-english locales (temporary calls to Sys.setenv(LANGUAGE = "en") for all tests related to vanilla (translated) R error messages)

always normalize exclusions in lint_dir normalize exclusions in read_settings wrt .lintr file location fix tests with language-dependent error messages adapt tests to new settings behaviour fix cache operations forcing read_settings - this caused test failures for exclusions. Previously, the incorrect normalization of exclusions let the tests pass because the files weren't matched to their exclusion entries in .lintr but now they do and care needs to be taken not to call read_settings() which would load the exclusions from lintr/.lintr

…rate error make sure entire file exclusions properly apply to Windows as well (otherwise .lintr wouldn't be respected for the offending file on Windows)

russHyde

Thanks for the PR, and for requesting my review. I like how you approached excluding directories. I've requested a couple of changes.

I started a PR that aimed to address the relative-path exclusions issue in #438. I'd like to check whether the tests that were written for that PR are all adressed in this PR before continuing.

tests/testthat/test-assignment_linter.R

tests/testthat/test-dir_linters.R

R/cache.R

R/exclude.R

@russHyde

original author: @russHyde

.lintr-ignore newly added test package in the main .lintr

AshesITR · 2020-11-06T17:53:20Z

Thank you @russHyde for taking the time to do a review.
I stole some files from your PR - I hope you don't mind.

AshesITR · 2020-11-06T17:58:30Z

tests/testthat/test-lint_package.R

+# These packages should not have a .lintr file:  Hardcoding a .lintr in a
+# dummy package throws problems during `R CMD check` (they are flagged as
+# hidden files, but can't be added to RBuildIgnore since they should be
+# available during `R CMD check` tests)


As a side note: Couldn't we override the options(lintr.linter_file = "test_dot_lintr") to overcome R CMD check warnings about hidden files?

Incorporated all suggestions

AshesITR · 2020-11-14T20:59:13Z

@russHyde do you have the time for another round of review?

russHyde · 2020-11-20T14:12:23Z

Okay. I'll look at it today

tests/testthat/dummy_packages/assignmentLinter/DESCRIPTION

russHyde · 2020-11-23T11:52:30Z

In it's current form, with a dummy package in "tests/testthat/package", if you run the following from the lintr root, the vignettes subdirectory is not excluded from linting:

lint_package("tests/testthat/package", exclusions = "tests/testthat/package/vignettes")

And, on setwding to the package root, and running:

lint_package(exclusions = "vignettes") // or lint_package(".", exclusions = "vignettes")

an error comes up: "Error in normalize_exclusions(c(exclusions, settings$exclusions), root = path, : object 'pattern' not found"

But, when a ".lintr" file is added into the "package" root with exclusions: "vignettes", the vignettes are excluded from linting.

Although the current PR does a good job of ensuring that directories are excluded when specified in the .lintr config, it doesn't appear to handle directory-exclusions that are passed via the exclusions = "..." arguments. This was an issue that meant my original PR for handling relative filepaths got really large.

I am happy for this PR to be merged, provided that you explicitly say that it handles directory exclusions that are specified in the .lintr config. If you do this, can you open a new issue about supporting directory exclusions via the "exclusions" argument to the lint_*() functions. (Maybe have a look at how @joaopmatias handled it in the unmerged russHyde#2 )

add parse_settings to lint_package() mainly for tests (suppressing global .lintr config exclusions)

AshesITR · 2020-11-23T12:12:24Z

Thanks @russHyde
I've moved the package.

The current behaviour in this PR would be that exclusions is always relative to the project root (so your example should do exclusions = "vignettes" from outside as well).

It's not hard to change this. What do you prefer?

russHyde · 2020-11-23T12:24:00Z

I'd really rather that that was done in a separate PR. We'd have to discuss whether files in ..., exclusions=..., are specified relative to the (possibly unspecified) project root, the (possibly absent) .lintr, the current directory, the targetted directory.

When exclusions are specified within the .lintr it seems a lot simpler: files should be specified relative to the directory containing the .lintr file or as full-paths.

AshesITR · 2020-11-23T12:25:22Z

Sounds good. Then this PR is done as far as I can tell.

AshesITR · 2020-11-24T13:34:15Z

ping @russHyde travis build is finally complete

russHyde

Just some minor changes. Happy to merge this without CI if these are addressed.

R/lint.R

NEWS.md

…_exclusions

AshesITR · 2020-11-25T19:08:55Z

@russHyde thanks for taking all the time to review this thoroughly.
I think we can merge now.

russHyde

LGTM

russHyde · 2020-11-25T20:22:22Z

Feel free to merge whenever. Many thanks for your hard work on this issue.

AshesITR requested a review from jimhester as a code owner October 21, 2020 19:18

AshesITR added 5 commits October 26, 2020 21:56

fix lintr::lint_package("lintr") failure due to test file with delibe…

3d9cd7f

…rate error make sure entire file exclusions properly apply to Windows as well (otherwise .lintr wouldn't be respected for the offending file on Windows)

Merge branch 'master' into feature/ignore-dirs

dbe6e61

Merge branch 'master' into feature/ignore-dirs

abc4076

update NEWS and documentation

a2f13a2

AshesITR changed the title ~~[WIP] support excluding entire directories~~ Support excluding entire directories Nov 1, 2020

AshesITR added 2 commits November 1, 2020 21:04

Merge remote-tracking branch 'upstream/master' into feature/ignore-dirs

6c02397

Merge branch 'master' into feature/ignore-dirs

2a2f11e

AshesITR requested a review from russHyde November 3, 2020 21:08

Merge branch 'master' into feature/ignore-dirs

77317c1

russHyde previously requested changes Nov 6, 2020

View reviewed changes

tests/testthat/test-assignment_linter.R Outdated Show resolved Hide resolved

tests/testthat/test-dir_linters.R Show resolved Hide resolved

R/cache.R Show resolved Hide resolved

R/exclude.R Show resolved Hide resolved

AshesITR added 5 commits November 6, 2020 17:58

move LANGUAGE = "en" to expect_lint()

34c6580

add internal documentation for normalize_exclusions()

5ecacb9

clarify purpose of delayed read_settings() calls in cache.R

6bc76a9

steal tests from lintr#439

fa40340

original author: @russHyde

add test for lint_dir(relative_path = FALSE)

b65e6fa

.lintr-ignore newly added test package in the main .lintr

fix typo

361355b

AshesITR commented Nov 6, 2020

View reviewed changes

AshesITR requested a review from russHyde November 6, 2020 19:10

fix R CMD check warnings (document(), wrong \link syntax, Suggest withr)

3a6262b

AshesITR added 2 commits November 12, 2020 15:59

Merge branch 'master' into feature/ignore-dirs

444a1ca

Merge branch 'master' into feature/ignore-dirs

b2a4125

Merge branch 'master' into feature/ignore-dirs

3f3442e

Merge branch 'master' into feature/ignore-dirs

4257484

russHyde requested changes Nov 23, 2020

View reviewed changes

tests/testthat/dummy_packages/assignmentLinter/DESCRIPTION Show resolved Hide resolved

move tests/testthat/package to tests/testthat/dummy_packages

c7af1ec

add parse_settings to lint_package() mainly for tests (suppressing global .lintr config exclusions)

AshesITR mentioned this pull request Nov 23, 2020

How should the exclusions = ... argument work? #580

Open

AshesITR requested a review from russHyde November 23, 2020 12:27

AshesITR added 2 commits November 23, 2020 21:58

document()

9f536d7

Merge branch 'master' into feature/ignore-dirs

4fae382

russHyde requested changes Nov 25, 2020

View reviewed changes

R/lint.R Outdated Show resolved Hide resolved

NEWS.md Outdated Show resolved Hide resolved

apply last changes, document(), test(), fix roxygen tag for normalize…

babe9f7

…_exclusions

AshesITR mentioned this pull request Nov 25, 2020

Who lints the linters? #584

Closed

russHyde approved these changes Nov 25, 2020

View reviewed changes

AshesITR merged commit f955476 into r-lib:master Nov 26, 2020

AshesITR deleted the feature/ignore-dirs branch November 26, 2020 07:26

AshesITR mentioned this pull request Nov 28, 2020

[WIP] - Exclude files by relative path (lint_[package|dir]) #439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support excluding entire directories #539

Support excluding entire directories #539

AshesITR commented Oct 21, 2020 •

edited

Loading

AshesITR commented Oct 23, 2020

AshesITR commented Oct 23, 2020

russHyde commented Oct 25, 2020

AshesITR commented Oct 26, 2020 •

edited

Loading

russHyde left a comment

AshesITR commented Nov 6, 2020

AshesITR Nov 6, 2020

AshesITR commented Nov 14, 2020

russHyde commented Nov 20, 2020

russHyde commented Nov 23, 2020 •

edited

Loading

AshesITR commented Nov 23, 2020

russHyde commented Nov 23, 2020

AshesITR commented Nov 23, 2020

AshesITR commented Nov 24, 2020

russHyde left a comment

AshesITR commented Nov 25, 2020

russHyde left a comment

russHyde commented Nov 25, 2020

Support excluding entire directories #539

Support excluding entire directories #539

Conversation

AshesITR commented Oct 21, 2020 • edited Loading

AshesITR commented Oct 23, 2020

AshesITR commented Oct 23, 2020

russHyde commented Oct 25, 2020

AshesITR commented Oct 26, 2020 • edited Loading

russHyde left a comment

Choose a reason for hiding this comment

AshesITR commented Nov 6, 2020

AshesITR Nov 6, 2020

Choose a reason for hiding this comment

AshesITR commented Nov 14, 2020

russHyde commented Nov 20, 2020

russHyde commented Nov 23, 2020 • edited Loading

AshesITR commented Nov 23, 2020

russHyde commented Nov 23, 2020

AshesITR commented Nov 23, 2020

AshesITR commented Nov 24, 2020

russHyde left a comment

Choose a reason for hiding this comment

AshesITR commented Nov 25, 2020

russHyde left a comment

Choose a reason for hiding this comment

russHyde commented Nov 25, 2020

AshesITR commented Oct 21, 2020 •

edited

Loading

AshesITR commented Oct 26, 2020 •

edited

Loading

russHyde commented Nov 23, 2020 •

edited

Loading