Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 25.86% 31.31% +5.45%
==========================================
Files 35 36 +1
Lines 1481 1587 +106
==========================================
+ Hits 383 497 +114
+ Misses 1098 1090 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| method(convert, list(class_character, class_pkg)) <- | ||
| function(from, to, ...) { | ||
| if (endsWith(tolower(from), ".rds")) { | ||
| convert(readRDS(from), class_pkg) | ||
| } else if (grepl("\\bPackage:", from[[1L]])) { | ||
| pkg_from_dcf(from, ...) | ||
| } else { | ||
| pkg(from, ...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Added a few extra handlers for converting a character string into a pkg object. Specifically ensuring that we can handle Rds file paths and DCF strings.
With these, whatever is passed to the report's package parameter, we can just run convert() on it to build a pkg() object and handle Rds paths, DCF strings and package names through a single interface.
| method(convert, list(class_list, class_pkg)) <- | ||
| function(from, to, ...) { |
There was a problem hiding this comment.
Similarly added handling for lists, which were effectively already handled inside the from_dcf method. Now it's just split into building a list from the DCF contents and then converting the list into a pkg object.
| nodes |> | ||
| xml2::xml_attr("href") |> | ||
| basename() |> | ||
| tools::file_path_sans_ext() |> | ||
| unique() |> | ||
| length() | ||
| paths <- xml2::xml_attr(nodes, "href") | ||
| filenames <- basename(paths) | ||
| filestems <- tools::file_path_sans_ext(filenames) |
There was a problem hiding this comment.
A bit of an unrelated fix, but noticed that the use of |> would force us to bump our minimum R version support so I just converted them into more traditional calls.
| capture <- evaluate::evaluate( | ||
| evaluate_fn, | ||
| stop_on_error = 1L, | ||
| debug = !isTRUE(quiet), | ||
| output_handler = evaluate::new_output_handler(value = identity) | ||
| ) |
There was a problem hiding this comment.
When logging is enabled we use evaluate to capture it.
It's off by default, since it's been a bit problematic in riskmetric; though here we're offloading all the nastiness of sinking output to a more mature package. Hopefully this is a bit more stable than what we had before, but still would prefer to keep it off by default until we have had an opportunity to stress test it a bit more.
| format.evaluate_evaluation <- function( | ||
| x, | ||
| ..., | ||
| style = c("text", "ansi", "html") | ||
| ) { | ||
| style <- match.arg(style) | ||
|
|
||
| if ( | ||
| identical(style, "html") && !requireNamespace("htmltools", quietly = TRUE) | ||
| ) { | ||
| stop( | ||
| "html formatting of evaluation logs requires suggested package ", | ||
| "`htmltools`" | ||
| ) | ||
| } | ||
|
|
||
| out <- utils::capture.output(evaluate::replay(x)) | ||
| switch( | ||
| style, | ||
| text = paste(cli::ansi_strip(out), collapse = "\n"), | ||
| ansi = paste(out, collapse = "\n"), | ||
| html = { | ||
| html <- cli::ansi_html(paste(out, collapse = "\n")) | ||
| htmltools::tags$div( | ||
| style = cli::ansi_html_style(colors = 8L), | ||
| htmltools::tags$pre(htmltools::HTML(html)) | ||
| ) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
To support prettier output of logs in the report, allow a style option that will hopefully make it easier to emit plain text or rich html reports.
jthompson-arcus
left a comment
jthompson-arcus
left a comment
There was a problem hiding this comment.
@dgkf focused on the logging as requested. I don't think any of this structure would pose an issue to the accompanying Shiny app.
I did have a couple comments below.
| knit_print.evaluate_evaluation <- function(x, ...) { | ||
| # nolint end | ||
| res <- format_output(evaluate::replay(x)) | ||
| if (!is.character(res) && requireNamespace("knitr", quietly = TRUE)) { | ||
| knitr::knit_print(res) | ||
| } else { | ||
| cat(res) | ||
| } | ||
| } |
There was a problem hiding this comment.
Added this in since the last review to accommodate printing during knitting.
With this we can have a block that is just:
pkg@logs$r_cmd_checkAnd we will get a richly formatted html output in html reports. Currently just html is handled, but we could handle others as well if we'd like rich logs.
|
Check what happens with logs when you have nested metric evaluation.
|
Added some tests for this. Fundamentally this is a behavior of In short, the logs capture is scoped so if one metric calls another the logs are attributed to the lowest metric being evaluated on the call stack at any given point. This means that regardless of order of execution, logs will be consistently captured. However, if you want to capture a full log of a parent, including the outputs of a child metric that might be evaluated in the middle of a parent metric, you'd need to cobble those together yourself. I think these situations will be unlikely/rare and it's more important that we have reproducible log capture. |
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
Looks good and test are well designed and work well.
class_source_archive_resource is not documented. I guess roxygen2 doesn't handle double assignments well.
See the note also to provide auto-completion to the objects.
Some vignettes use suggested packages without checking that the package is installed (it failed with rosv). On that note, even after installing I get an error:
r <- cran_repo_resource("haven", "0.1.1", repo = "https://cloud.r-project.org/")
p <- pkg(r, permissions = "network")
p$rosv_vulnerability_df
<S7_error_method_not_found: Can't find method for generic `pkg_data_info(field, resource)`:
- field : S3<pkg_data_field_rosv_vulnerability_df/pkg_data_field>
- resource: MISSING>| # RStudio, when trying to produce completions,will try to evaluate our lazy | ||
| # list elements. Intercept those calls and return only the existing values. | ||
| if (is_rs_rpc_get_completions_call()) { |
There was a problem hiding this comment.
Couldn't stop myself, to point out that maybe we need to check if the code is run by RStudio and perhaps check if with Positron or VSCode something similar happens (I doubt it).
Another related approach would be to provide a .DollarNames and .AtNames to provide default R autocompletion as we see fit.
There was a problem hiding this comment.
Ah, good call on the other IDEs. I haven't tested in Positron/VSCode.
We do provide .DollarNames. I think the problem is that RStudio tries to determine the class of each value for its completion menu. By evaluating the object it prompts metric evaluation, which stalls the completion dialogue.
There was a problem hiding this comment.
Sorry, I didn't check if it was available already. That method looks weird why do it use :: ? AFAIK, we don't need name-spacing when registering it and a simple #' @export should be enough provided that the function is .DollarNames.<class> but that might be related to the S7 class (I didn't find this documented on any article)
There was a problem hiding this comment.
That method looks weird
Yeah, I agree with you there! I think there are a few weird things going on.
why do it use
::?
This is because S7 class objects have a class name of <package>::<class>. Here val.meter::pkg is a class name.
and a simple
#' @exportshould be enough
I can try it again. I think I had some issues with it picking up the generic name because it starts with a period.
| function(pkg, resource, field, ...) { | ||
| # package installs use `system2()` whose output cannot be captured by sink() | ||
| # so we just execute quietly | ||
| covr::package_coverage(resource@path, type = "tests", quiet = TRUE) |
There was a problem hiding this comment.
Again not the point of the PR but should code coverage cover all? Or at least we should provide a way to change that.
| covr::package_coverage(resource@path, type = "tests", quiet = TRUE) | |
| covr::package_coverage(resource@path, type = "all", quiet = TRUE) |
There was a problem hiding this comment.
Yeah, good call. I'd say we should break these off the other types into other metrics.
| ) | ||
| } | ||
|
|
||
| old_options <- options(width = 80L, crayon.enabled = TRUE, cli.ansi = TRUE) |
There was a problem hiding this comment.
On a different function (capture_pkg_data_derive()) some other options were used:
original_opts <- options(
width = 80L,
crayon.enabled = TRUE,
cli.ansi = TRUE,
cli.dynamic = FALSE,
cli.num_colors = 256L
)I think it would be a nice bet to get a default_opts or internal_opts for reusing across the package.
There was a problem hiding this comment.
Ah, I like that idea. It could be an option, eg
options(val.meter.log.opts = list(
width = 80L,
crayon.enabled = TRUE,
cli.ansi = TRUE,
cli.dynamic = FALSE,
cli.num_colors = 256L
))Configured with a default so that they're overwritable if anyone wants something other than these.
Co-authored-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>
github-project-automation
Bot
moved this from In Progress
to Done
in Package Development
4 participants
I spent some time before the holidays familiarizing myself with
val.reportand made a few changes toval.meterthat I think will help streamline the reporting process.I had a few goals in mind:
packageparameter to be more agnostic. Ideally, we should be able to pass in a pre-calculatedpkgobject (by Rds path), a DCF-formatted string of metrics (for example, from our repository), or a package name to be calculated on the fly.Will annotate the changes inline in comments.
@jthompson-arcus - let me know what you think of the logs capture. I know this was a contentious feature in
riskmetricand would be curious to hear your thoughts on this approach.@llrs-roche - would love to get your thoughts on some of the reporting-oriented changes.