Skip to content

feat: rewrite ojo_use_template to use new okpolicy-report-template - #23

Merged
spindouken merged 6 commits into
mainfrom
feat-rewrite-ojo_use_template-to-use-new-okpolicy-report-template
May 4, 2026
Merged

spindouken merged 6 commits into
mainfrom
feat-rewrite-ojo_use_template-to-use-new-okpolicy-report-template

Conversation

@spindouken

Copy link
Copy Markdown
Contributor

We have updated the Quarto templates for reports and website creation, so we need to update the wrapper function accordingly.

  • brought in cli_yeah function from odcrscraper/utils.R to handle cli communication
  • update main path management as well as quarto package path handling
  • implemented .interactive from rlang for automated environments
  • swapped system calls for processx for stderr, stdout capturing
  • added cli styling aesthetics for all alerts and error messages
  • tryCatch to handle quarto and system failures
  • graceful aborts with error messages
  • currently defaulting to echoing stdout, command, and R errors to CLI as well (see tryCatch block)

- change from base_dir/project_name to single path parameter
- add kebab-case validation for project names
- create directory automatically if missing
- abort if directory exists and is non-empty
- extract cli_yeah() to cli.R for reuse
@brancengregory

Copy link
Copy Markdown
Member

Since this is out first review, just a reminder to push back on any place where I'm overlooking the reasoning behind a change or overriding something in a way you think is a bad idea. This is also going to be way more verbose than in the future because I'm trying to show you my line of thought.

Overall this is an extremely good PR from the start. It worked out of the box and explained exactly how to use it. Comments were minimal and all the ones there I found useful and necessary.

If there were/are changes to make that take longer to implement that for me to comment on what they are I would kick them back to you but so far everything has been so small that I made the change myself and just want to provide some reasoning in addition to you reviewing the diff of what I changed. I'm also making changes in smallish commits so you can follow easier.

  • I added {quarto} and {processx} to package dependencies and removed the check for whether quarto R package was installed. I view these as reasonable dependencies and not bloat.
  • I kept the vast majority of your logic because it was fire, but changed the main argument back to path
    • This was to maintain backwards compatibility but also for user ergonomics. I would expect to be able to call ojo_use_template("inst/reports/new-report")
  • I added report naming validation for kebab, lower case, alphanumeric pattern, pending @anthonyokc 's agreement
    • I didn't think about this as something we would want until playing with it
  • I moved the cli_yeah helper to a new cli.R file

- validate template with rlang::arg_match()
- map friendly names to github specs internally
@brancengregory

Copy link
Copy Markdown
Member

Added another quality of life thing so we don't have to remember the format for the template. The tradeoff is we can't use arbitrary quarto template paths but I think this is reasonable since our goal is just for our templates

@brancengregory

Copy link
Copy Markdown
Member

Also didn't know I'd want this but added full interactive mode where it will prompt for the arguments

@brancengregory

Copy link
Copy Markdown
Member

Failing R-CMD-check is expected and okay. It's an arrow related CI hell trap we aren't going down at this time

@spindouken
spindouken merged commit 3a760bf into main May 4, 2026
5 of 6 checks passed
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.

feat: rewrite ojo_use_template to use new okpolicy-report-template

3 participants