Skip to content
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

Add error on no method for class in cases where we use default #92

Closed
athowes opened this issue Jun 7, 2024 · 7 comments
Closed

Add error on no method for class in cases where we use default #92

athowes opened this issue Jun 7, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@athowes
Copy link
Collaborator

athowes commented Jun 7, 2024

For the epidist_prepare and epidist generics we use the default method to do something. For example to transform the data to the right class. Or to run the model without having the same generic for every model class. Here we need a different pattern to implement an error when you provide something of the wrong class. See #91.

@athowes
Copy link
Collaborator Author

athowes commented Jul 10, 2024

With PR #125 then the epidist_prepare function will be removed.

So the only case where we have this pattern is in epidist.

I think there should be a good way to handle this in a one off pattern. Something like a warning unless the class of the object is from a specific list. Yes this means upkeeping this list but I don't think it's that bad. i.e.

stopifnot(inherits(..., c("possible1", "possible2"))

@seabbs
Copy link
Contributor

seabbs commented Jul 10, 2024

I think we can instead dispatch on something where the default method is a warning?

@athowes
Copy link
Collaborator Author

athowes commented Jul 10, 2024

Don't we then lose the default epidist working for all the models?

So we'd need:

  1. Default method giving warning
  2. Model 1 method
  3. Model 2 method
  4. Model 3 method
  5. ...

Of course this is also fine but involves maintaining a method for each model right? Perhaps we might need that in the end when the models are different anyway.

@athowes
Copy link
Collaborator Author

athowes commented Jul 11, 2024

Perhaps you mean like

function
function.default is warning
function.model1 no warning
function.model2 no warning

Isn't this a more complicated way to do stopifnot(inherits(..., c("possible1", "possible2"))?

Thoughts @seabbs? Think this could be a quick PR to get off our plate.

@seabbs
Copy link
Contributor

seabbs commented Jul 11, 2024

Its more complicated but it avoids hard coding a list into a function (which is bad as it prevents extension by a user). I think #151 would resolve this?

@athowes
Copy link
Collaborator Author

athowes commented Jul 11, 2024

Yep #151 would solve it I think, good point. The validate function has a default that says no model

@athowes
Copy link
Collaborator Author

athowes commented Jul 18, 2024

Closed via #151.

@athowes athowes closed this as completed Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants