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

russHyde · 2019-12-16T15:23:50Z

fix #438 - pin exclusions relative to dir/pkg in lint_[package|dir]

When calling lint_package(path), any excluded-files specified in a
.lintr file were originally handled relative to the working directory.
This meant

lint_package("myPackage") # called from the parent of myPackage
    
lint_package(".") # called from inside myPackage

gave different results for excluded files (the former including files
that were supposed to be excluded).

Ensure read_settings handles exclusions when passed a directory

read_settings was modified: when it is passed a directory, any
excluded files that are specified in a user config are now specified
relative to the directory (eg, the package root), rather relative to the
current working directory

Added 'dir_prefix' argument to normalize_exclusions

normalize_exclusions is used to specify filepaths of the excluded
files; When running lint_dir or lint_package, the directory /
package-root is now passed to normalize_exclusions & excluded files are
pinned relative to this.

Added a test to ensure lint_package(path) handles excluded files

A couple of tests were added. These set up a temporary package with
lint-containing source files, run lint_package() from outside and
inside the temp package (+/- exclusions).

Exclusions of the form list(filename = lines_to_omit) and list(filename)
are both included in the test.

When calling lint_package(path), any excluded-files specified in a .lintr file were originally handled relative to the working directory. This meant ~~~ lint_package("myPackage") # from the parent of myPackage lint_package(".") # from inside myPackage ~~~ gave different results for excluded files (the former including files that were supposed to be excluded). A couple of tests were added. These set up a temporary package with lint-containing source files, run lint_package() from outside and inside the temp package (+/- exclusions). Exclusions of the form list(filename = lines_to_omit) and list(filename) are both included in the test.

`normalize_exclusions` is used to specify filepaths of the excluded files; When running lint_dir or lint_package, the directory / package-root is now passed to normalize_exclusions & excluded files are pinned relative to this.

`read_settings` was modified: when it is passed a directory, any excluded files that are specified in a user config are now specified relative to the directory (eg, the package root), rather relative to the current working directory

russHyde · 2019-12-16T16:51:26Z

Failure appears to be due to the use of package.skeleton to make a temporary package during the tests; restricted to the devel branch of R. I'll try and work out if there's a simpler way to do that test

jimhester · 2019-12-16T17:25:22Z

I would suggest you make the dummy package in the tests/testthat directory rather than creating it during the test.

joaopmatias · 2019-12-16T19:54:35Z

R/exclude.R

+    names(x) <- file.path(dir_prefix, names(x))
+  }
  if (normalize_path) {
    x <- x[file.exists(names(x))]       # remove exclusions for non-existing files


I think it would be best if
x <- x[file.exists(names(x))]
were outside the if statement.

joaopmatias · 2019-12-16T20:56:31Z

Thank you for tackling this issue @russHyde !

I'd also like to point out that the use of normalize_exclusions in lint_package should use the parameter normalize_path = TRUE.
This is due to the facts that read_settings always produces exclusions with absolute paths and the variable files in lint_dir also uses absolute paths. As a result, the parameter normalize_path in normalize_exclusions could be dropped.

Finally, in lint_dir, the exclusions coming out of read_settings are being ignored if parse_settings = TRUE. At least, that's what I infer from looking at lint and exclude.

.lintr files in test-subdirs cause problems in R CMD check (and can't be added to .RbuildIgnore because they are needed during tests)

Wasn't able to include .lintr files in a dummy-package in a subdir of `tests/`. Instead, during tests we copy a dummy package from `tests/testthat/dummy_packages/<pkgName>` into a temp-dir and add a .lintr file to the temp package during the test.

russHyde · 2019-12-17T11:14:06Z

@jimhester

I've updated the tests.
A simple package was added as a subdir of tests/testthat/dummy_packages/.
Problems with R CMD check meant that dummy-packages that contain a .lintr file couldn't be included in the dummy-packages folder (check baulks at the hidden files; and the .lintr files in these subdirs can't be added to the lintr RBuildIgnore, since they wouldn't then be available during the tests in R CMD check)
As such, the lint_package integration tests copy the dummy package to a temp dir, and then optionally add a .lintr config for use during testing.

Would you welcome another PR that allows exclusion of all elements in a directory? This doesn't seem to be covered at present

joaopmatias · 2019-12-17T17:18:48Z

R/settings.R

+      # - `filename` means exclude all lines in `filename`
+      # normalise_exclusions needs to know which directory the excluded-files
+      # are pinned against
+      dir_prefix <- if (is_directory(filename)) filename else NULL


In my opinion, the behavior is still ambiguous if filename is not NULL and points to a file.

Could we use dirname in that case? Or would we want to assign value <- NULL in that case?

Let me know if that should not be thought of in this PR.

I'm not really sure what the problem is. I've strived not to affect how linting works when lint() is called on single files.
But yeah, if lint() is called on a single file, then any exclusions defined in the config may not apply; so I guess syntastic, or similar, might flag lints when you're looking at a file that 'should be' excluded from linting. (but if files other than the file-of-interest are present in the exclusion list, there doesn't seem much point to NULL those out of the exlcusions list (given that you aren't linting them))

(Sorry if I missed your point)

My concern is due to the fact that lint(filename) looks at the configurations in a .lintr file in the same folder of filename.

So, if filename is an actual path (e.g. inst/example/bad.R), then read_settings might extract exclusions: list("bad.R") (assume we create a file inst/example/.lintr with that content) and, in this situation, the results of

> lint("bad.R")

and

> lint("inst/example/bad.R")

will be different (assume the commands are run with the right workdirs).

Looking at this part of the code in both executions we see that dir_prefix = NULL and value = list("bad.R"). Then, since the workdirs are different, the function normalize_exclusions will find the file bad.R in one case and not in the other, and the outputs will be different.

My suggestion, would be to make dir_prefix equal to the path to the folder containing filename. E.g. one could change it to (or something nicer)

dir_prefix <- if (is_directory(filename)) filename else {if (is.null(filename)) NULL else dirname(filename)}

EDIT:
After reading your comment with more attention it seems to me that value should be made equal to NULL or empty list().

joaopmatias · 2019-12-17T17:20:45Z

In my opinion, the changes look good.

jimhester · 2019-12-17T19:00:14Z

Rather than copying the whole temporary package, maybe just create the needed .lintr file, then clean it up with on.exit()?

russHyde · 2019-12-17T21:04:32Z

Thanks @jimhester I hadn't considered that.

I'm in two minds about this pr. Though I'll update the tests, please don't merge it yet.

`assignmentLinter` is a more informative package name for the dummy package used in lint_package integration tests.

When running tests on dummy packages, rather than optionally adding .lintr configs to a temp-copy of the package, add the config to the existing package and remove it `on.exit()`.

…subdirectory (vs pkg-root / outside package)

russHyde · 2019-12-18T10:08:45Z

Packages have a prescribed structure.

<pkg>
    |- R
    |- tests
    |- .lintr
    //--snip--//

It makes sense for the lintr-config to list excluded files with-respect-to the package root (and the code I've modified allows this)

The changes made here might cause confusion for more general directories / project structures.
They might also cause confusion when using lint or lint_dir within a typical package structure

As a simple example:

<pkg>
    |- R
        |- abc.R ["abc = 123\nghi <- 456\n"]
        |- jkl.R
    |- .lintr ["exclusions: list('R/abc.R')"]

Assignment lints in "R/abc.R" are flagged (despite the .lintr file having excluded that file) if:

lint_dir() or lint_dir("R") are called from the package root
lint("abc.R") is called from the "R" directory

lint("R/abc.R") does not flag lints when called from the package root

If a .lintr is specified (in a package, R-project or more general directory structure), it might be better to parse exclusions with respect to the directory in which the .lintr is found (than wrt the current working dir, as present; or an assumed package structure, as I've tried to implement here)

…ing dir and of position of .lintr

russHyde · 2019-12-19T14:30:56Z

I've pushed up a few (currently failing) tests:

lint(file) should be independent of the working directory; and
lint(file) should be independent of whether the .lintr is put in the same dir as file or in the project root (provided the filepaths for exclusions are adjusted)

joaopmatias · 2019-12-19T14:53:01Z

Hi @russHyde
I believe that the second point in your last comment is counterproductive: if the file paths for exclusions have to be adjusted, then, in a sense, lint(file) is dependent of the working dir since its requiring us to change .lintr (in the case that it's in the folder of file) whenever we change the working dir.
I hope this makes sense.

I'll submit a draft PR later as well just to try to get my idea across better.

russHyde · 2019-12-19T15:21:48Z

I don't see why the working directory has any relation to my second point.

The excluded files should be encoded in .lintr consistent with their position relative to .lintr.

My second point is illustrated below. If I have

<project>
- .lintr [exclusions: list('scripts/excluded.R')]
- scripts
    - included.R
    - excluded.R

It should give the same results as this:

<project>
- scripts
    - .lintr [exclusions: list("excluded.R")]
    - included.R
    - excluded.R

... and in both cases, the results should be independent of the working directory.

…to the .lintr-containing directory

russHyde · 2019-12-19T18:26:41Z

So, I've modified the code again.

Now any exclusions that are specified in a .lintr file are handled relative to the directory that contains .lintr (so the .lintr-specified exclusions should be independent of the working directory and also be insensitive to the .lintr being specified in the source-file-directory or the project-root)

An issue has arisen though: when a user calls lint(..., exclusions = "some_file") the files that they exclude in the function call should be pinned against the current working directory (unlike those in .lintr). This looks straightforward to fix.

So, this would not exclude the first line from "/scripts/excluded.R", yet.

<project>
- .lintr [exclusions: list()]
- scripts
    - included.R
    - excluded.R

R> setwd("scripts")
R> lint("excluded.R", exclusions = list('excluded.R' = 1))

I haven't attempted to handle .lintr files in the user's home directory yet

russHyde · 2019-12-19T18:28:58Z

[I don't understand why the tests have passed on Travis. There are a number of tests in test-exclusions.R that fail on my machine]

joaopmatias · 2019-12-20T10:46:12Z

I got the same thing @russHyde . Amongst others, it's showing an failure at line 192 of test-exclusions.R, but only when I run devtools::test(). When I run the test file separately, the test passes.

Then, I removed tests/exclusions/testthat/exclusions-test from .lintr, and that made devools::test() pass. I'll give more feedback when I have time to look into it.

joaopmatias · 2019-12-22T20:53:53Z

I figured why the tests were not passing.

Turns out I wasn't completely right, when I said that lint() only searches for a .lintr file in the folder containing the file to be linted. In the case, that there are none it keeps searching for a .lintr file in the parent folders. As a last resort, it searches in the home directory of the system.

This is my interpretation from reading the functions find_config and find_config2.

I would suggest eliminating find_config2 and reducing find_config greatly.

Finally, I did a PR to the PR branch with my suggestions. Please, let me know what you think @russHyde , @jimhester

EDIT: if we want to allow search for a configuration file in parent folders, then exclusion-test should be removed from the file .lintr in the root of the project.

AshesITR · 2020-11-28T20:01:29Z

Closing this since the feature was implemented in #539 and AFAIK all salvageable code from this (namely, the tests) has been merged.

russHyde added 3 commits December 16, 2019 15:12

add 'dir_prefix' argument to normalize_exclusions

a6dda3f

`normalize_exclusions` is used to specify filepaths of the excluded files; When running lint_dir or lint_package, the directory / package-root is now passed to normalize_exclusions & excluded files are pinned relative to this.

russHyde requested a review from jimhester as a code owner December 16, 2019 15:23

add 'withr' to suggests, since used in a test

91bca33

joaopmatias reviewed Dec 16, 2019

View reviewed changes

russHyde added 3 commits December 17, 2019 10:54

add dummy-packages for lint_package integration tests

4ed901f

remove .lintr-containing dummy package

0a925d9

.lintr files in test-subdirs cause problems in R CMD check (and can't be added to .RbuildIgnore because they are needed during tests)

remove subdirectory .lintr files from .RBuildIgnore

96e3e68

joaopmatias reviewed Dec 17, 2019

View reviewed changes

russHyde added 3 commits December 18, 2019 09:25

rename dummy package to reflect the type of lint

39e980c

`assignmentLinter` is a more informative package name for the dummy package used in lint_package integration tests.

write & remove a .lintr config to dummy package

15c6aaf

When running tests on dummy packages, rather than optionally adding .lintr configs to a temp-copy of the package, add the config to the existing package and remove it `on.exit()`.

check that lints are the same when lint_package is called from a pkg-…

639ed94

…subdirectory (vs pkg-root / outside package)

russHyde added 2 commits December 18, 2019 10:12

clarify a comment

b4a508e

add integration tests for lint(): lints should be independent of work…

848fa8d

…ing dir and of position of .lintr

russHyde changed the title ~~fix #438 - Exclude files by relative path (lint_[package|dir])~~ [WIP] - Exclude files by relative path (lint_[package|dir]) Dec 19, 2019

russHyde added 2 commits December 19, 2019 18:19

expand test

6b445b0

when a .lintr config is defined, excluded files are treated relative …

71b4d0d

…to the .lintr-containing directory

AshesITR mentioned this pull request Oct 23, 2020

Support excluding entire directories #539

Merged

3 tasks

AshesITR closed this Nov 28, 2020

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

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

Uh oh!

Conversation

russHyde commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russHyde commented Dec 16, 2019

Uh oh!

jimhester commented Dec 16, 2019

Uh oh!

joaopmatias Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

joaopmatias commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russHyde commented Dec 17, 2019

Uh oh!

joaopmatias Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

russHyde Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

joaopmatias Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaopmatias commented Dec 17, 2019

Uh oh!

jimhester commented Dec 17, 2019

Uh oh!

russHyde commented Dec 17, 2019

Uh oh!

russHyde commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russHyde commented Dec 19, 2019

Uh oh!

joaopmatias commented Dec 19, 2019

Uh oh!

russHyde commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russHyde commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russHyde commented Dec 19, 2019

Uh oh!

joaopmatias commented Dec 20, 2019

Uh oh!

joaopmatias commented Dec 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AshesITR commented Nov 28, 2020

Uh oh!

Uh oh!

russHyde commented Dec 16, 2019 •

edited

Loading

joaopmatias commented Dec 16, 2019 •

edited

Loading

joaopmatias Dec 17, 2019 •

edited

Loading

russHyde commented Dec 18, 2019 •

edited

Loading

russHyde commented Dec 19, 2019 •

edited

Loading

russHyde commented Dec 19, 2019 •

edited

Loading

joaopmatias commented Dec 22, 2019 •

edited

Loading