Feat/check up to date #338
Conversation
| @@ -205,27 +187,6 @@ config_log_entry <- function(path, | |||
| config[out_fields] | |||
| } | |||
|
|
|||
| #' Compare a file to an MD5 sum | |||
There was a problem hiding this comment.
this moved to utils.R
| @@ -56,7 +56,7 @@ test_that("new_model() throws an error if the model file does not exist", { | |||
| }) | |||
|
|
|||
| test_that("compare read_model() and new_model() objects", { | |||
| temp_mod_path <- create_temp_model(YAML_TEST_FILE) | |||
There was a problem hiding this comment.
this is unrelated cleanup
seth127
force-pushed
the
refactor/new-model-stan-prep
branch
from
5c05a69
to
f3ac959
Compare
… file_matches_string()
…e(). Need to decide what to do about NA though. When the file is not there, should we say T, F, or NA?
…tests for it. Also changed all of test-check-up-to-date.R tests to check for the expected messages.
seth127
force-pushed
the
feat/check_up_to_date
branch
from
5db0e02
to
e286aa7
Compare
| } | ||
|
|
||
| #' @export | ||
| check_up_to_date.bbi_nonmem_model <- function(.bbi_object, ...) { |
There was a problem hiding this comment.
check_up_to_date.default <- function(.bbi_object, ...) {
Right now, the bbi_nonmem_summary and bbi_nonmem_model methods are identical so maybe consider just setting this as the default method.
There was a problem hiding this comment.
Part of the point of this is prepare for adding Stan stuff so, even though these are the same, they're not gonna be the default methods. I considered adding another parent class that they would share, but that seemed a little excessive to have something like
class(.mod)
# "bbi_nonmem_model" "bbi_nonmem_object" "bbi_model" "list"("bbi_model" is already the parent class that they'll share with Stan and other kinds of models)
Anyway, that would be a totally valid way to do it, but I opted for just duplicating a couple dispatches instead (and made them use the same private helper to avoid copy/paste).
| if(isTRUE(any_changes)) { | ||
| message(paste( | ||
| glue("The following files have changed in {get_model_id(.mod)}"), | ||
| paste("*", names(which(changed_files)), collapse = "\n"), |
There was a problem hiding this comment.
maybe this is just a me thing but it's kinda weird to mix paste and glue.. especially because glue has the same base functionality as paste. I.e. this could be rewritten:
message(glue(
glue("The following files have changed in {get_model_id(.mod)}"),
glue_collapse("*", names(which(changed_files)), sep = "\n"),
sep = "\n"
))
paste works so this isn't wrong I just don't see why we are using it if we have already imported the glue package
There was a problem hiding this comment.
yea, I see that. I always just use paste unless I explicitly need the {} glue thing. I would change it here but... I do this all over the place so consistency is really not within reach.
closes #339
Also refactored
config_log()to call the new function internally.