Add support for externally scored packages #70
Conversation
kyleam
force-pushed
the
external
branch
2 times, most recently
from
e80f0c0 to
2776da4
Compare
The test of format_scores_for_render() constructs the set of expected criteria from the object read from *.scorecard.json, tweaking for the renames of the testing criteria. This makes it difficult for a reader to know what's expected. Reduce the amount of code logic in the test by replacing this logic with a hard-coded list of expected criteria.
The scorecard template uses values like "r NULL" and "`r NA`" for the parameter defaults. If these values aren't specified during the rmarkdown::render(), they come through as strings (e.g., "r NULL" and "`r NA`"), not the intended R objects. At the moment, this doesn't matter because all the values are supplied to the rmarkdown::render() call. But that will change when support for externally scored packages is added and dep_versions_df is left unspecified. Set the defaults to all these values to null, allowing downstream R code to check whether the parameter is specified with is.null().
The dependency table, at least as coded, is very tied to R. An upcoming commit will add support for rendering a scorecard for non-R packages. Place the guard in format_dependency_versions() rather than the scorecard-template.Rmd block for consistency with format_comments() and format_traceability_matrix().
format_appendix() and format_scores_for_render() rely on the coverage values coming in rounded. An upcoming commit will allow rendering a scorecard for non-R packages, with coverage values coming from an external source. In that case, we can't expect the values to come in rounded, and, in any case, it makes sense for rendering code to be responsible for the rendered format. With this change, the values are always rendered to two decimal places (e.g., "100.00%" instead of "100%"). That has the advantage of nicer alignment in the table.
An upcoming commit will add support for externally scored packages, and test_dirs won't provided for that input.
'covr' is tied to a particular package and language. Instead choose a name that is based on the result type.
An upcoming commit will add support for externally scored non-R packages, in which case 'R Script' doesn't work for the column name. We could switch from 'R script' to something else at runtime base on the type of package being scored. However, 'Code File' seems appropriate enough for the coverage result of any type of package, so go with that to avoid unnecessary branching in the code and divergence in the report.
Use a key that can also work for the coverage data coming in from externally scored non-R packages.
format_appendix() will also process coverage data from non-R packages, so choose a name that reflects that.
The 'R CMD check' score factors in the numbers of notes/warnings/errors in addition to whether the tests passed or failed, so it's a score in the 0 to 1 range. However, package types other than R will likely only have a pass/fail, in which case it doesn't make sense to display the specific score.
Formulas prevent 'R CMD check' from detecting issues like undefined variables. Use a standard anonymous function instead.
format_traceability_matrix() selects columns for text wrapping with a range. Instead use a vector, which makes it easier for the reader to know which columns were intended and makes it less likely to behave unexpectedly if the data frame structure changes.
If either sys or env_vars is missing, format_metadata() errors with Error in t.default(unlist(NULL)) : argument is not a matrix To prepare for the upcoming introduction of the externally scored input, rework format_metadata() to not require sys and env_vars and to abort if date or executor isn't specified.
An upcoming commit will add support for non-R packages. In practice, we can detect an R package versus non-R package by looking at the files in the results directory, but we might as well explicitly record a type when dumping the scores for an R package.
Version the on-disk format. If we need to do a major rework of the format, this provides a mechanism to signal the change and to detect older formats.
results_dir is fed directly to get_result_path(), so NULL is not okay: > get_result_path(NULL, "scorecard.pdf") Error in basename(out_dir) : a character vector argument expected
This note isn't relevant for the user. The *.export_doc.rds file is not something that they should ever write without the help of make_traceability_matrix(), and we do not advertise the underlying structure. And with upcoming support for externally scored package, a user-defined matrix can come in through a YAML file.
Externally scored packages are going to pass in different parameters. Move the parameter preparation for "internally" scored packages outside of render_scorecard() so that render_scorecard() can handle both internally or externally scored packages by selecting which param function to call.
|
Not sure if this was discussed elsewhere, but was there any thought about adding support for externally scored packages to to |
| scorecard_type <- params[["pkg_scores"]][["scorecard_type"]] | ||
| if (is.null(scorecard_type)) { | ||
| stop("pkg_scores unexpectedly does not have 'scorecard_type' field") | ||
| } |
There was a problem hiding this comment.
I took a look at the format_traceability_matrix() changes too; specifically noting how the report changes for R packages` versus the external ones ('command' vs 'exported function').
Was there any consideration about more clearly denoting the scorecard_type within the report?
There was a problem hiding this comment.
Was there any consideration about more clearly denoting the
scorecard_typewithin the report?
I'm open to suggestions. I personally don't see the value/need to show that at the report level.
There was a problem hiding this comment.
I'm open to suggestions
I was thinking of maybe modifying one of the first few sentences to just more explicitly state the software type, but I think @seth127 would be better suited to chime in here. Was mostly wondering if it might be desirable for QA folks.
There was a problem hiding this comment.
modifying one of the first few sentences to just more explicitly state the software type
Somewhat related to that: I did look through the PDF for parts that seemed too tied to the R context, but I didn't spot anything. Please let me know if you do.
There was a problem hiding this comment.
This is an interesting question. At this point, I don't see a clear right-or-wrong answer, but I'll continue to mull it over.
In absence of a reason to add it, I would say to leave it as is.
Only to the extent of "I don't think we need to deal with that now". We don't have a use case at the moment (whereas for R packages, there's MPN). My preference would be to leave it be unless we think something here seems like a step in the wrong direction if we later want to add support. But it's a good point, and I should at least 1) document that it's not supported and 2) make sure the error is reasonable if those are called with an external results directory. |
There was a problem hiding this comment.
This is looking good (sorry for the disorganized review). Left a couple comments but the technical changes look good to me.
Though exporting local_create_external_results might be overkill, it would be nice to provide more support for setting this up, especially given the required file naming conventions.
- If it were me, and I was testing this package out from outside Metrum (not setting up a pipeline), I would want to start with a template and manually adjust the scores & names to get the report im looking for. Would be nice to have some 'create external template' helper. Perhaps not worth worrying about, but just wanted to give me two cents.
Will re-review after the other documentation changes discussed above have been added.
| local_create_external_results <- function(pattern = "mpn-scorecard-tests-", | ||
| .local_envir = parent.frame(), | ||
| clean = TRUE) { | ||
| tdir <- withr::local_tempdir( | ||
| pattern = pattern, .local_envir = .local_envir, clean = clean | ||
| ) |
There was a problem hiding this comment.
Could export local_create_external_results() (perhaps with different name), and reference in external_scores documentation to see an example. Just thinking out loud
There was a problem hiding this comment.
Thanks for the suggestion. My preference at this point would be to avoid adding something to the NAMESPACE that has unclear value. If that usefulness becomes clear later, it's easy to move/rename/export.
Though exporting
local_create_external_resultsmight be overkill, it would be nice to provide more support for setting this up, especially given the required file naming conventions.
I don't think these are files that anyone would want to set up by hand (with the exception of the traceability yaml). You'd always have tooling to generate these automatically.
| wrap_text(.x, width = 24, indent = TRUE, strict = TRUE)), | ||
| dplyr::across( | ||
| all_of(c(entry_name, "code_file", "documentation")), | ||
| function(x) wrap_text(x, width = 24, indent = TRUE, strict = TRUE) |
There was a problem hiding this comment.
I don't think this is quite the right section of the code for this comment, but it seems close enough...
I noticed that, in the Traceability Matrix, the text-wrapping on the Documentation column looks nicer than in the Test Files column. Specifically, it attempts to split on separators and indents the wrapped lines. For example:
I'm assuming there's a reason for this, but if it's easy enough to fix, can we make the Test Files column have the nicer wrapping too?
This is a bit of a nit pick though, so if there's an implementation reason why this is difficult, it's really not necessary to spend much time on it.
There was a problem hiding this comment.
I'm assuming there's a reason for this, but if it's easy enough to fix, can we make the Test Files column have the nicer wrapping too?
I don't know what the original rationale was (hopefully @barrettk can chime in), but, when working on gh-69, I recall wondering a bit about it and then realizing that indenting, as currently implemented by wrap_text, shouldn't be applied because that would lead to confusing indentation for the remaining files. (Basically, I think it's meant only for scalar values.)
Example with indent = TRUE and default wrap_sym characters:
diff --git a/R/format-report.R b/R/format-report.R
index 1b24d0a..7b248fb 100644
--- a/R/format-report.R
+++ b/R/format-report.R
@@ -730,7 +730,7 @@ format_traceability_matrix <- function(
),
# Tests can be longer due to page width (pg_width) settings (we make it wider)
test_files = purrr::map_chr(.data$test_files, function(tests){
- wrap_text(tests, width = 40, strict = TRUE, wrap_sym = NULL)
+ wrap_text(tests, width = 40, indent = TRUE, strict = TRUE)
})
)
}
Still, I think it'd be an improvement to at least wrap on the default wrap_sym characters (at least in this example):
diff --git a/R/format-report.R b/R/format-report.R
index 1b24d0a..e6d120f 100644
--- a/R/format-report.R
+++ b/R/format-report.R
@@ -730,7 +730,7 @@ format_traceability_matrix <- function(
),
# Tests can be longer due to page width (pg_width) settings (we make it wider)
test_files = purrr::map_chr(.data$test_files, function(tests){
- wrap_text(tests, width = 40, strict = TRUE, wrap_sym = NULL)
+ wrap_text(tests, width = 40, strict = TRUE)
})
)
}
There was a problem hiding this comment.
I looked over the PR discussion and commit messages in the original wrap_text PR (gh-57), and I didn't spot any discussion of why test_files uses wrap_sym = NULL.
I'll add a commit here that drops wrap_sym = NULL.
There was a problem hiding this comment.
I believe wrap_sym = NULL was set for test files to avoid the issue you highlighted above (caused too many new lines that made it harder to read). Indents were not meant to be made for additional elements in a vector (i.e. multiple test files), but rather only when a newline was introduced to due to the column width. Could be worth adjusting in the future but I think your second screenshot looks good
With recent commits, all of the formatting functions have been decoupled, at least conditionally, from the assumption that it is an R package that's being scored. Define a set of files that any type of package can use to render a scorecard. The structure of the results directory for external scores is similar to that of the directory for "internal" scores. check.txt replaces check.rds, coverage.json replaces covr.rds, and matrix.yaml replaces export_doc.rds. For scorecard.json, an external _could_ write that file. But doing so is awkward because that file includes overall scores derived from other scores in the file. To compute those, the external scorer would need to either duplicate the scoring logic or depend on mpn.scorecard. So instead have the external scorer produce parts of scorecard.json, and make mpn.scorecard responsible for stitching those parts together at render time. A notable limitation of the external input is that the check values must be 0 or 1; 0 through 1, like is done for R packages, is not supported. There's a number of ways we could add support for this, but punt on that given that all our initial non-R packages of interest need only a pass/fail result.
For some externally scored packages, it is not be feasible to calculate coverage. Don't bother making coverage optional for internally scored R packages, because in that case we'll try to calculate coverage with covr and, at the very least, record the error.
For bbi, two code files exceed the column width, leading to them being
truncated and, for reasons I don't understand, the heading for the
next column ("Test Coverage") being split into two lines.
Based on trial and error, all of bbi's files fit in the column when
wrapped to a width of 45, including bbi's 45-character
"parsers/nmparser/parse_parameter_structure.go".
format_appendix() has a good amount of conditional handling. Add a test to make sure the covr failure arm isn't unintentionally made unreachable by upstream changes.
External scorers are responsible for providing the *.coverage.json, and that file can have an overall coverage score but not per file scores. If there are no rows in the coverage table, replace the table with a note to make it clear that this is expected rather than a rendering error. Thanks to @seth127 for the suggestion. Re: #70 (comment)
When wrapping the entry point, code file, and documentation file, format_traceability_matrix() uses wrap_text's default wrap_sym value so that breaks are made at "/", "_", and "-" to get under the width. If those splits fail to get the string under the width, then the file is split at any character. For tests files, `wrap_sym = NULL` is passed, so wrap_text() goes straight for splitting at any character, leading to entries like .../foo/bar/baz_tes t.go Switch to preferring splits at "/", "_", and "-" so that intra "word" breaks are less likely. .../foo/bar/ baz_test.go Re: #70 (comment)
render_scorecard_summary() and summarize_package_results() have not been reworked to support external scores. We use these functions for MPN. For non-R packages, we don't yet have a need for these functions. Document that these functions don't accept external scores, and add a custom guard so that a more informative error is given if any results directory with external scores is passed.
|
Updates:
range-diff |
For bbi, the "o" in parsers/nmparser/parse_parameter_comments.go (44 characters) is partially truncated. Decrease the width by one to prevent this case, and, given these aren't fixed-width fonts, by one more to hopefully provide enough margin to avoid this in other cases.
This series teaches
render_scorecard()how to render a scorecard from a set of files created by an external scorer (i.e. something other thanscore_pkg()). These files capture much of the same information as*.scorecard.jsonand the various RDS files.Series overview
The main feature change comes with commit 22 of the series. The preceding commits are prep commits, as well as minor tweaks and fixes in the area. Most of these are internal facing. I'll highlight commits that change user-facing details or are otherwise worth special attention.
Commit 5 tweaks how coverage values are rendered. In particular, there will now always be two decimal places (e.g.,
100.00%where the old approach would have100%). I think that consistency is an improvement.Commit 9 renames the coverage 'R Script' column to 'Code file', even for R packages. I think that works, but it may be worth sticking with 'R Script' for R packages to minimize the presentation changes.
Commits 22 and 23 introduce the main feature.
This last commit is needed to prevent a couple of code files in the bbi repo from getting truncated in the coverage table. I'm choosing the wrap width by trial and error, but, if there's some systematic way to do that based on the page and column width values, please let me know. (That seems unlikely, especially because it's not a fixed-width font.)
Testing notes
You can use the new
local_create_external_results()helper to create a directory of external results for testing.I'll also send results for bbi and pkgr out of band.