Skip to content

Conversation

@joaopmatias
Copy link

I would like to draw the attention to some changes that could be useful.

  • Removed test-exclusions from .lintr.

  • Changed arguments of one instance of normalize_exclusions, where part of the input was already in the form of absolute paths.

  • Changed normalize_exclusions to prevent adding a prefix when the paths in the exclusions are absolute paths.

Prevented from adding directory prefixes to full paths. Adjusted exclusion prefixes in lint_package. Removed exclusions-test from .lintr.
@russHyde
Copy link
Owner

russHyde commented Jan 9, 2020

Thanks for this @joaopmatias . I won't have time to focus on this branch for a few days. Do you have a test that can distinguish the changes made to normalize_exclusions?

@joaopmatias
Copy link
Author

Hi @russHyde ! Yes, I can make a unit test that distinguishes those changes soon.

@joaopmatias
Copy link
Author

Hi @russHyde ! I added some tests covering the changes involving the function normalize_exclusions

@joaopmatias joaopmatias requested a review from russHyde February 1, 2020 22:43
@russHyde
Copy link
Owner

russHyde commented Feb 3, 2020

Hi. Sorry I haven't been more responsive. This PR (the combination of our changes) is a bit bigger than I expected and life got in the way. I'll try to review it properly this week.

@joaopmatias
Copy link
Author

Hi! That's fine by me. The changes definitely seem to be plenty compared to the expected change in behavior, but they are looking very good, in my opinion.

@russHyde
Copy link
Owner

russHyde commented Feb 5, 2020

Thanks. I appreciate that you think the changes are looking good, but you haven't provided much explanation here re

  • what the changes hope to achieve;
  • and why the changes that were made achieve that goal.

For example, why was the test-exclusions file removed from the .lintr config and how does that connect to the aims of the pull request?

So I'll have to do a bit of guesswork before I can go forward.

@joaopmatias
Copy link
Author

joaopmatias commented Feb 7, 2020

Let me try to answer your questions.

  • what the changes hope to achieve;

The changes affect the function normalize_exclusions so that, whenever provided, it only adds a path prefix in the case that it is given a relative path.

Additionally, files described with a relative path in the argument exclusions of the function lint_package will not be ignored.

Finally, test-exclusions was removed from .lintr so that the tests in test-exclusions.R > context("exclude") > test_that("it excludes properly", and in test-expect_lint.R > context("expect_lint") > test_that("multiple checks", do pass.

  • and why the changes that were made achieve that goal.

The changes to the function normalize_exclusions were due to the fact that this function would always add a path prefix if that argument were provided. However, there may be instances where this function receives both absolute and relative paths, such as in the line exclusions <- ... in the function lint_package.

The changes to the function lint_package were due to the fact that in the line lint <- lintdir(... the exclusions given as relative paths were ignored.

The changes to .lintr were due to the fact that lint in some tests of the package would show no bad patterns for test-exclusions.

@joaopmatias
Copy link
Author

joaopmatias commented Feb 7, 2020

I only noticed that .lintr did not have to be changed while answering your questions.

I will add a commit reverting .lintr back to the original format, and specify the exclusions argument in the tests that I wanted to make pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants