metrumresearchgroup / bbr

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

Improve `delete_models()` messages #560

Closed seth127 closed 1 year ago

seth127 commented 1 year ago

delete_models() was originally conceived for cleaning up models created by test_threads() and for this reason it defaults to .tags = "test threads". This is fine, but when attempted to delete an arbitrary model, you see this:

> delete_models(mod107)
The following tags were not found:
- test threads 

Error in delete_models(mod107) : None of specified tags were found

This makes sense, but we could probably add a more helpful message about that being the default value and that users should pass .tags = NULL to delete arbitrary models, regardless of tag.

Also, when the user does pass .tags = NULL they get this message about NA

> delete_models(mod107, .tags = NULL)
Are you sure you want to remove 1 models with the following tags?: `NA` (Yes/no/cancel) yes
Removed 1 models with the following tags:
- NA

That message could also be better. I'm not sure what the right message is in that case, but maybe just not mentioned tags at all (if the user passes .tags = NULL).

barrettk commented 1 year ago

@seth127 FWIW the main reason for the NA tag was in the event people deleted multiple models at once, where one did not have tags:

> delete_models(c(mod106, mod107), .tags = NULL)
Are you sure you want to remove 2 models with the following tags?: `tag 1, `NA` (Yes/no/cancel) yes
Removed 2 models with the following tags:
- tag1
- NA

Just want to make sure that whatever message we change a non-tagged model to, it also reads well when combined with models that do have tags.

seth127 commented 1 year ago

Ah good point Kyle. Per my original comment:

That message could also be better. I'm not sure what the right message is in that case, but maybe just not mentioned tags at all (if the user passes .tags = NULL).

I think I would lean toward not mentioning tags at all if the user passes .tags = NULL. Maybe something like Are you sure you want to remove <n> models (ignoring tags)?

What do you think of that?