feat: ✨ conversion log schema comparison#301
Conversation
So both use `register_log` to show that they expect the log for a single register.
So it's aligned with the generic `print()`'s param
| #' @param register_log A tibble returned by [convert()], filtered to only | ||
| #' contain rows from a single register. |
There was a problem hiding this comment.
This function doesn't require only a single register, why the change here?
| #' sas_file <- fs::path_package("fastreg", "extdata", "test.sas7bdat") | ||
| #' conversion_log <- convert(sas_file, output_dir = fs::path_temp("output")) | ||
| #' log_schema(conversion_log) | ||
| log_schema <- function(register_log) { |
There was a problem hiding this comment.
This function isn't doing the action of logging. Could you use another verb here? Based on comment below, this could be print_log() instead.
| #' sas_file <- fs::path_package("fastreg", "extdata", "test.sas7bdat") | ||
| #' conversion_log <- convert(sas_file, output_dir = fs::path_temp("output")) | ||
| #' log_schema(conversion_log) |> print() | ||
| print.fastreg_schema <- function(x, ...) { |
There was a problem hiding this comment.
Nice use of an S3 😁 But, could this not be moved into the function above and then that function could be called print_log()? I'm not sure it is necessary to use S3 here. Would log_schema() be used for anything else other than printing the differences/etc?
| knitr::kable() |> | ||
| print() | ||
| if (length(x$diff_tables) > 0) { | ||
| cat("### Schema differences\n\n") |
There was a problem hiding this comment.
| cat("### Schema differences\n\n") | |
| cat("\n\n### Schema differences\n\n") |
Since the previous table is right above this header, so needs more empty lines above it.
| #' conversion_log <- convert(sas_file, output_dir = fs::path_temp("output")) | ||
| #' log_schema(conversion_log) |> print() | ||
| print.fastreg_schema <- function(x, ...) { | ||
| cat(x$description, "\n\n") |
There was a problem hiding this comment.
| cat(x$description, "\n\n") | |
| cat(x$description) |
It looks like knitr::kable puts new lines above the table by default.
| #' conversion_log <- convert(sas_file, output_dir = fs::path_temp("output")) | ||
| #' log_schema(conversion_log) |> print() | ||
| print.fastreg_schema <- function(x, ...) { | ||
| cat(x$description, "\n\n") |
There was a problem hiding this comment.
Maybe rather than using cat() everywhere, you create a character vector with all the items in that single variable, than at the end print that.
| #' the reference schema. If there's many files with schema differences, | ||
| #' there will be multiple tables, for printing purposes. | ||
| #' | ||
| #' @importFrom rlang .data |
There was a problem hiding this comment.
This should be placed in a file called fastreg-package.R made by usethis::use_package_doc(). All imports like that are then put in that file to keep track of them.
github-project-automation
Bot
moved this from In review
to In progress
in Platform development
2 participants
Description
This adds an exported
log_schema()function with quite a few helpers. I've tried implementing it so it outputs a S3 object with a specifiedprint()method, so it can be called in theconversion-log.qmdin the same way aslog_as_table().Looking forward to feedback 😁
Closes #287
Needs a thorough review.
Checklist
just run-all