-
Notifications
You must be signed in to change notification settings - Fork 5
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
Issue #372: Refactor formula dispatch #383
Conversation
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.
Really nice. One small comment about opening up the default method now that this is optional.
Actually we have the same issue in #' Default method for defining a model specific family
#'
#' @inheritParams epidist_family_model
#' @param ... Additional arguments passed to method.
#' @family family
#' @export
epidist_family_model.default <- function(data, ...) {
cli_abort(
"No epidist_family_model method implemented for the class ", class(data),
"\n", "See methods(epidist_family_model) for available methods"
)
} Can fix both here. Edit: confirming that in |
…la (already check for model being in the right class in as_model() function)
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.
Nice. LGTM
Description
This draft PR will close #372.
I am considering doing #382 as well since #372 involves reorganising the function locations. In doing so it seems easy to also fix the other function locations. I'd like to move quickly.
What has been done:
On #372:
epidist_formula
is now a regular function which contains all processing that happens across modelsepidist_formula_model
which dispatches on the class ofdata
into the model specific partformula.R
and the tests are moved into atest-formula.R
fileOn #382:
formula.R
Edit: I'm going to let this get to passing tests and if it's quick and I can catch a reviewer I'll put the #382 into a new PR.
Checklist