-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature/coverage by node #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm comfortable with the code changes from an execution perspective (i.e. what they actually do), but want to discuss the organization a bit. See my comments
@@ -96,7 +97,25 @@ PackageFunctionReporter <- R6::R6Class( | |||
|
|||
# TODO [patrick.bouer@uptake.com]: Implement packageTestCoverage metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove this TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bburns632 !!
, by = "node" | ||
, all.x = TRUE) | ||
|
||
log_info(msg = "Done calculating package coverage...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is the "done" message, it doesn't need the ...
#' @importFrom assertthat assert_that is.string | ||
#' @importFrom covr package_coverage tally_coverage | ||
#' @importFrom data.table as.data.table setnames | ||
#' @importFrom methods is | ||
#' @return A list of instantiated packageReporters the user can then interact with | ||
#' @export | ||
CreatePackageReport <- function(packageName | ||
, packageReporters = DefaultReporters()) { | ||
, packageReporters = DefaultReporters() | ||
, packagePath = NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that instead of sewing this throughout CreatePackageReport
, we should create a PackageCoverageReporter
that gets initialized with this packagePath
and implements package_test_coverage
.
That's a lot cleaner than exposing this argument and passing it around just for this one operation. To allow people to color nodes by coverage, we could support some more general way to join in data (like package coverage)
I'd like @patrick-boueri to weigh in on this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the defacto interface and single entry point. I agree we shouldn't overload it with args, but i don't see this going wild beyond this. in principle if we add a ton more, then yeah it should be encapsulated and passed as an object, but i do think it's really useful to have a single function call that does a lot of sane sensible things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure fair enough
James agreed this was not going wild with the params
This uses
covr
to calculate coverage by function and add the coverage ratios to the nodes ofPackageFunctionReporter
object. Coverage is only calculated with a package path is given. This code checks that a valid path is given.