Skip to content

Adding a few knitr helpers#6

Merged
dgkf merged 12 commits into
mainfrom
4-1-knitr-extras
Feb 13, 2026
Merged

Adding a few knitr helpers#6
dgkf merged 12 commits into
mainfrom
4-1-knitr-extras

Conversation

@dgkf

@dgkf dgkf commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

Changes broken out from #5

Adds the two knitr helpers, knitr_mutable_header and knitr_logger with 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.

@dgkf dgkf force-pushed the 4-1-knitr-extras branch from 73dcf83 to d979556 Compare January 27, 2026 15:56
Comment thread R/reporter.R
Comment thread inst/report/package/pkg_template.qmd
@dgkf dgkf requested a review from llrs-roche January 27, 2026 16:01

@llrs-roche llrs-roche left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread R/utils_knitr.R
Comment thread R/utils_knitr.R
Comment thread R/utils_knitr.R Outdated
Comment thread R/utils_knitr.R
@dgkf

dgkf commented Jan 28, 2026

Copy link
Copy Markdown
Contributor Author

All really great feedback. Will implement all your suggestions.

if we export it we should be able to maintain these functions well

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.

@dgkf dgkf left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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

Comment thread R/utils_knitr.R
@dgkf dgkf requested a review from llrs-roche January 29, 2026 19:19

@llrs-roche llrs-roche left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@llrs-roche

Copy link
Copy Markdown
Collaborator

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

@llrs-roche llrs-roche linked an issue Feb 5, 2026 that may be closed by this pull request
@dgkf dgkf mentioned this pull request Feb 5, 2026
Comment thread R/session.R
@dgkf dgkf requested a review from llrs-roche February 12, 2026 22:17
@dgkf

dgkf commented Feb 12, 2026

Copy link
Copy Markdown
Contributor Author

Okay, got the ubuntu checks passing. Since the others are also failing on main, I propose that we focus on fixing the other OSes in a follow-up PR. @llrs-roche let me know if that works for you.

@llrs-roche llrs-roche left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agreed! Let's move forward even if the package is broken at Windows and macOS at the moment.
Feel free to merge

@dgkf dgkf merged commit 217d7fa into main Feb 13, 2026
4 of 8 checks passed
@dgkf dgkf deleted the 4-1-knitr-extras branch February 13, 2026 16:50
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.

Update report to use val.meter

2 participants