-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add argument for log_write to export log object as Rds #164
Changes from 10 commits
2bdfdae
055c741
29867bb
fe6c61c
8ac8cb4
8f7f9bc
2bf9ed8
317f9ce
5fa319f
80152b4
8dccc88
03a5162
6d5cc28
a046e13
fdfbd33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,6 +150,8 @@ log_cleanup <- function() { | |
#' to a log file | ||
#' | ||
#' @param file String. Path to file executed | ||
#' @param include_rds Boolean. Option to export log object as Rds file. | ||
#' Defaults to FALSE | ||
#' @param remove_log_object Boolean. Should the log object be removed after | ||
#' writing the log file? Defaults to TRUE | ||
#' @param to_report String vector. Objects to optionally report; additional | ||
|
@@ -177,6 +179,7 @@ log_cleanup <- function() { | |
#' log_write(file) | ||
log_write <- function(file = NA, | ||
remove_log_object = TRUE, | ||
include_rds = FALSE, | ||
to_report = c("messages", "output", "result")){ | ||
# Set end time and run time | ||
set_log_element("end_time", strftime(Sys.time(), usetz = TRUE)) | ||
|
@@ -285,8 +288,39 @@ log_write <- function(file = NA, | |
write_log_element("log_path", "Log path: ")) | ||
|
||
writeLines(cleaned_log_vec, | ||
con = file.path(get_log_element("log_path"), | ||
get_log_element("log_name"))) | ||
con = file.path(get_log_element("log_path"), | ||
get_log_element("log_name"))) | ||
if (include_rds){ | ||
rds_fields <- c( | ||
"end_time", "start_time", "run_time", "user", "hash_sum", | ||
"log_path", "log_name", "file_path", "file_name", | ||
"unapproved_packages_functions", "errors", "warnings" | ||
) | ||
log_options <- as.list(getOption('log.rx')) | ||
cleaned_log_list <- purrr::map2( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't 100% convinced myself, but I'm wondering if we would just return everything to the rds rather than NULL out sections. Those writing the log and those consuming these rds files might be different people with different use cases. |
||
log_options, | ||
names(log_options), | ||
function(i, x){ | ||
if(x %in% c("messages", "output", "result")){ | ||
if(x %in% to_report){ | ||
return(i) | ||
} | ||
} else if(x %in% c(names(log_cleanup()), rds_fields)){ | ||
return(i) | ||
} | ||
} | ||
) | ||
cleaned_log_list$session_info <- session_info(info = "all") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you have to do this because we capture the output of session_info and store it, rather than just storing the session info object. I think we can move the capture output to the write function so this will still print pretty, and then you won't need to get the session info again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. Adjusted and updated the unit test. |
||
saveRDS(cleaned_log_list, | ||
file = file.path( | ||
get_log_element("log_path"), | ||
paste0(tools::file_path_sans_ext( | ||
get_log_element("log_name") | ||
),".Rds") | ||
) | ||
) | ||
} | ||
|
||
if (remove_log_object) { | ||
log_remove() | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to upversion to 0.3.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parmsam-pfizer can you update NEWS to include this under 0.3.0 and resolve the merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks!