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

Adding function lintr_dir to check R files for given path #360

Merged
merged 23 commits into from
Sep 27, 2019

Conversation

arekbee
Copy link
Contributor

@arekbee arekbee commented Dec 18, 2018

No description provided.

@peterskipper
Copy link

My team could definitely use this if it was added!

@mattyb
Copy link
Contributor

mattyb commented Jan 15, 2019

I'm seeing similar test failures on a different branch

@frankschmitt
Copy link

This would be tremendously useful. Is there anything I can do to help to get this pull request merged into master?

@jimhester
Copy link
Member

The main reason I haven't merged it is it duplicates logic that would be better off refactored into a function that could be used here and in lint_package().

@arekbee
Copy link
Contributor Author

arekbee commented Feb 12, 2019

@jimhester lint_package() has been refactored.
There are no integration tests for lint_dir nor lint_package. Is it required?

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #360 into master will increase coverage by 1.29%.
The diff coverage is 61.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #360      +/-   ##
==========================================
+ Coverage   81.74%   83.03%   +1.29%     
==========================================
  Files          46       46              
  Lines        1972     1975       +3     
==========================================
+ Hits         1612     1640      +28     
+ Misses        360      335      -25
Impacted Files Coverage Δ
R/lint.R 87.34% <61.9%> (+15.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 994ab77...edf6030. Read the comment docs.

R/lint.R Outdated
file %in% names(exclusions) && exclusions[[file]] == Inf
},
logical(1))
function(i) {

Choose a reason for hiding this comment

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

@arekbee I think this file will benefit from styling with styler package - https://github.com/r-lib/styler

this will make review easier.

R/lint.R Outdated
#' Apply one or more linters to all of the R files in a package.
#' @param path the path to the base directory of the package, if \code{NULL},
#' Apply one or more linters to all of the R files in a directory
#' @param path the path to the base directory, if \code{NULL},
Copy link
Collaborator

@russHyde russHyde Aug 23, 2019

Choose a reason for hiding this comment

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

Could you check what actually happens when NULL is passed as the base-path, from the code it looks like the function will die (because dir(NULL) will)

function(x) {
x$filename <- re_substitutes(x$filename, rex(path, one_of("/", "\\")), "")
x
})
attr(lints, "path") <- path
}
Copy link
Collaborator

@russHyde russHyde Aug 23, 2019

Choose a reason for hiding this comment

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

None of the logic in lines 162-189 has changed; IMO, although styling tools are very useful, their use only simplifies PR review when applied consistently. Here, styler has made it look like more lines have changed than really have been changed, because it had been applied to code that hadn't previously been 'styler'ed. (there's no point changing the code back to its previous state, though)

R/lint.R Outdated
#' @param ... additional arguments passed to \code{\link{lint}}, e.g.
#' \code{cache} or \code{linters}.
#' @param exclusions exclusions for \code{\link{exclude}}, relative to the
#' package path.
Copy link
Collaborator

@russHyde russHyde Aug 23, 2019

Choose a reason for hiding this comment

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

The definitions for path, relative_path, ... and exclusions are almost identical to their definitions for lint_dir. You could use @inheritParams lint_dir to deduplicate the docs and rewrite the docs for lint_dir to apply generally for both lint_dir and lint_package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @russHyde , thank you for the tip. I haven't used @inheritParams yet.

@@ -198,4 +208,30 @@ test_that("it excludes properly", {
}
})

test_that("it excludes properly by dir", {
read_settings(NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid I don't understand what's happening in these tests. Should the user need to use read_settings() before using lint_dir? Could you add some comments to explain what is being setup and what is being tested when these tests run, please.

@@ -1,4 +1,14 @@
context("knitr_formats")

test_that("it handles dir", {
lints <- lint_dir(path = "knitr_formats", pattern = rex::rex(".R", one_of("html", "md", "nw", "rst", "tex", "txt")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps rex::one_of

@arekbee
Copy link
Contributor Author

arekbee commented Sep 14, 2019

How can I increase codecov/patch from 66.66% to 81,74% ??
Is it really needed (codecov/patch)?

jimhester
jimhester previously approved these changes Sep 27, 2019
@jimhester jimhester merged commit 89a4b4e into r-lib:master Sep 27, 2019
@jimhester
Copy link
Member

Great work, this package is now better thanks to you!

Thanks!! You are da bomb!

@arekbee arekbee deleted the lint_dir branch September 30, 2019 20:36
@ginberg ginberg mentioned this pull request Dec 6, 2020
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.

None yet

7 participants