Conversation
73dcf83 to
d979556
Compare
llrs-roche
left a comment
There was a problem hiding this comment.
Thanks for the smaller PR!
I left some comments here and there but haven't run the code yet (will update add more comments once I do).
Now that I think of it, if we export it we should be able to maintain these functions well. We will need examples and tests for them.
|
All really great feedback. Will implement all your suggestions.
This in particular is something I was debating. I'd prefer not to require internal functions in the chunks of the report, but I also see these functions as "internal" (at least subject to change and evolution, I don't really want to sign us up for maintaining a stable API for these) So I guess I see this package as a convenience for bundling packages functions used in the template, rather than a public-facing package for broader use. I'm not exactly sure how to manage function exports for that use case. |
…itr/rmarkdown is used
dgkf
left a comment
There was a problem hiding this comment.
@llrs-roche I think I addressed all your feedback.
- added tests
- added a note about use of exported helpers (but would be happy to revise and just keep them as non-exported if you think that would be better)
- updated docs and added examples
| roxygen2_knitr_note <- function() { | ||
| sprintf( | ||
| paste( | ||
| "`%s` `knitr` utilities are exported for use in", | ||
| "reports maintained by the R Validation Hub.", | ||
| "If you choose to use these functions for other purposes, be aware", | ||
| "that these are not considered stable for broader use." | ||
| ), | ||
| packageName() | ||
| ) | ||
| } |
There was a problem hiding this comment.
I'm still debating how best to handle these utilities so that they can be used in a quarto report without treating them like user-facing exports. If we decide to keep them as exports, I added this note that gets re-used as a \note{} section for each knitr helper.
Of course, the other option is to just keep them internal and use them as internal functions in the report... maybe that's the best option anyways.
Curious to hear your thoughts.
There was a problem hiding this comment.
I think they are features of the package. If the package only contains a template it is not really needed. The template could be just shared somewhere and it would work for the users.
However, I'm familiar with what is usually done with reporting tools/documents.
We could export them but not show them on pkgdown website or manual via @noRd if R CMD check allows it
llrs-roche
left a comment
There was a problem hiding this comment.
Locally checks failed:
Error (test-utils-knitr.R:127:3): knitr_mutable_header can modify yaml frontmatter during runtime
<badUnicodeHex/parseError/error/condition>
Error: '\U' used without hex digits in character string (<text>:14:19)
Error (test-utils-knitr.R:112:5): knitr_update_options accepts complex R objects through quarto::quarto_render
Error in `quarto::quarto_render(example_qmd, execute_params = params_expr, quiet = TRUE)`: ! Error running quarto CLI from R.
Caused by error in `quarto::quarto_render()`:
x Error returned by quarto CLI.
i Rerun with `quiet = FALSE` to see the full error message.
Error (test-utils-knitr.R:71:5): knitr_update_options accepts complex R objects through rmarkdown::render
<badUnicodeHex/parseError/error/condition>
Error: '\U' used without hex digits in character string (<text>:14:19)
Failure (test-report.R:12:5): Run test for the report / should be generated in all formats
Expected `package_report(...)` not to throw any errors.
Actually got a <rlang_error> with message:
[ FAIL 5 | WARN 0 | SKIP 0 | PASS 33 ]
Maybe because I'm on a Windows and
quarto::check_newer_version()
## ℹ You are using the latest stable version of Quarto: 1.8.27.
|
Maybe we need to have also windows checks? It is not the first time something works on someone else computer but not on my windows |
Changes broken out from #5
Adds the two
knitrhelpers,knitr_mutable_headerandknitr_loggerwith some minimal changes to the report to accommodate them.Affected files got formatted by
air, so I suggest checking the ignore whitespace changes box while reviewing.