metrumresearchgroup / bbr

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

add open_model_file #570

Closed kyleam closed 1 year ago

kyleam commented 1 year ago

This PR brings in the file_edit() helper from bbr.bayes and adds the "open control stream" helper proposed in #398 (related bbr.bayes discussion).

This doesn't come with any automated tests (or associated validation doc updates) because I don't have any good ideas for how to meaningfully test this, but please let me know if you have any suggestions. I've tested it manually in RStudio on model paths, model objects, and model summary objects.


From gh-398:

might want to look into what file.edit() does when you're not in an interactive Rstudio session (i.e. in the terminal or in an Rmd that gets knit)

file.edit doesn't play well with a non-interactive session:

image

Given the interactive/convenience nature of file.edit() and the open_model_file wrapper, that behavior seems okay to me. @seth127, do you think we should add special handling for the non-interactive context?

kyleam commented 1 year ago

The plan is to rewrite bbr.bayes to use this. I've pushed another branch that merges this PR's branch into bbr-bayes-split. I'll open a draft PR in the bbr.bayes repo that pulls that branch in.

update steps:

seth127 commented 1 year ago

Thanks for doing this Kyle.

Given the interactive/convenience nature of file.edit() and the open_model_file wrapper, that behavior seems okay to me. @seth127, do you think we should add special handling for the non-interactive context?

I do think it would be nice if we could handle this more gracefully (i.e. maybe just return NULL). My thinking is that people will likely put this code into things like RMD docs and then might later knit that without remembering to comment them out.

What do you think about just putting a if (!interactive()) return(invisible(NULL)) at the top of our file_edit()? Is there a downside to that?

kyleam commented 1 year ago

What do you think about just putting a if (!interactive()) return(invisible(NULL)) at the top of our file_edit()? Is there a downside to that?

Encouraging people to leave function calls in their Rmd that shouldn't be there :). But, no, I don't see any real downsides. I'll make that change. Are you okay with also giving a warning? (edit: never mind, I'll just go with silent return)

kyleam commented 1 year ago

@seth127 Added interactive() guard. Should be set for review (assuming checks pass).