Fixes package #3
Conversation
dominik-appsilon
left a comment
dominik-appsilon
left a comment
There was a problem hiding this comment.
Hey, thanks for the changes! Almost everything looks fine, I have just one major doubt about logic of checking paths in package_report() -- see the comment.
In general, I fell like operations on files in R/reporter.R would use some refactoring and overall cleanup. One of the particular issues I had was that due to missing dependencies report generation failed and temporary files got generated but not removed and this caused subsequent calls to package_report() to fail as well (multiple .qmd files). However, this is rather not something that should be worked on in this particular PR, just something to note for the future.
During testing I found one additional issue with test intests/testthat/test-render_table.R. It reads assessment from {riskreport} which, if not installed, fails. I assume it should be changed to {val.report}. But while at it -- do we use this .rds file only for testing? Do we want to store it with package? We could also mock it instead.
| if (!is.null(params$assessment_path)) { | ||
| params$assessment_path <- normalizePath(params$assessment_path, mustWork = TRUE) | ||
| params$package <- normalizePath(params$package, mustWork = FALSE, winslash = "/") | ||
| if (!is.null(params$assessment_path) || !nzchar(params$assessment_path)) { |
There was a problem hiding this comment.
Shouldn't we use && here instead of ||? If params$assessment_path is not NULL but it is string of length 0 the expression evaluates to TRUE which throws an error in normalizePath.
| gfm: | ||
| html-math-method: webtex | ||
| typst: | ||
| pdf: |
There was a problem hiding this comment.
Can I ask for the rationale behind moving away from typst? I haven't been involved in this discussion so I am curious.
There was a problem hiding this comment.
Is this something we want to have stored in the git repository?
Incomplete PR to fix some issues with the package. It aims to:
Fixes: #1