metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

nm_table_files crumps when no table files are found #467

Closed kylebaron closed 2 years ago

kylebaron commented 2 years ago

While refactoring this: the function should check if there are any table blocks and stop with an appropriate error message if no.

nm_table_files <- function(.mod, .check_exists = TRUE) {
  .p <- get_model_path(.mod)
  .l <- parse_ctl_to_list(.p)
  out_dir <- get_output_dir(.mod, .check_exists = .check_exists)

  # get file names from table statements and construct paths
  .f <- .l[names(.l) == "TABLE"] %>%
    map_chr(~paste(.x, collapse = " ")) %>%
    str_extract("\\bFILE\\s*=\\s*([^ ]+)") %>%
    str_replace("\\bFILE\\s*=\\s*", "") %>%
    str_replace("^\\.\\/", "") %>%
    file.path(out_dir, .)
barrettk commented 2 years ago

@seth127 What do you think of this? Could simply add something like this right below the code above:

  if(rlang::is_empty(.f)){
    stop("No table files were found.")
  }

Could add a more informative error message though

seth127 commented 2 years ago

I think that makes sense. I looked to see where we call it, looks like only in nm_tables() and nm_join().

Screen Shot 2022-03-04 at 3 26 36 PM

I was thinking through if this should be a warning instead, i.e. if there are any cases where downstream you would just want it to return the empty character vector, but I can't think of any. Seems like in any of these cases, you are trying to get the table files pulled in so, if it doesn't find any, you would rather have it just error and tell you that.

barrettk commented 2 years ago

Yeah was thinking the same thing and came to the same conclusion. The error will still be spit out even if its part of a nested function so we should be good. Committed that change in the same PR: https://github.com/metrumresearchgroup/bbr/pull/466

seth127 commented 2 years ago

As for the error message, I might just add the model file name, so they know where to look. You'll have the path already within that function as .p so just stop(glue("No table files were found in {.p}"))

kylebaron commented 2 years ago

This is what you get now:

> nm_join("../model/xyz/008")
Reading data file: .......-simple.csv                                                                                   
  rows: 31907
  cols: 11

 Error in nm_file.character(.files[.i]) : 
  Assertion on '.mod' failed: May not be NA. 
seth127 commented 2 years ago

yea, that's definitely worse. Ok, well it looks like this is fixed in #466 right? If so @barrettk you can close this when that PR merges to develop.

barrettk commented 2 years ago

Oh shoot, merged without seeing that comment. Will re-commit and ask for a review again