add butcher methods for all int_conform methods - #194
Conversation
|
|
||
| # @export - onLoad | ||
| #' @rdname inf_conformal-butcher | ||
| axe_call.int_conformal_full <- function(x, verbose = FALSE, ...) { |
There was a problem hiding this comment.
I know that there is a good amount of repetition. However the functions are also really short, so I think that this is the more readable form
There was a problem hiding this comment.
I don't mind but maybe they should all share a int_conformal class?
hfrick
left a comment
hfrick
left a comment
There was a problem hiding this comment.
Generally on board but I'm hoping to learn something about S3 registration details!
|
|
||
| # @export - onLoad | ||
| #' @rdname inf_conformal-butcher | ||
| axe_call.int_conformal_full <- function(x, verbose = FALSE, ...) { |
There was a problem hiding this comment.
I don't mind but maybe they should all share a int_conformal class?
| add_butcher_class(x) | ||
| } | ||
|
|
||
| # @export - onLoad |
There was a problem hiding this comment.
What's the reason for not directly exporting the method? The help page for vctrs::s3_register() says it's for registering an S3 method for a generic from a package you dont' import, just suggest. probably is importing butcher so I'm wondering if we need this extra layer here?
There was a problem hiding this comment.
I'm doing it the same way that it was done in {workflows}
It didn't work when I did it the normal way. So it might be a little out of my depth to understand why.
There was a problem hiding this comment.
I'm fairly sure that the reason for this in workflows is because butcher is only in Suggests and not in Imports.
Maybe that affects things here because we make use of the workflows methods, which don't seem to be exported from workflows? 🤔 I'm okay moving on and getting this merged though!
|
I assume that the reason why there isn't one overall class is that some of the methods have a I do believe it would be worth looking into if there is a way to unify some of the code. But I don't think this PR is the place to do it |
|
oh totally agree, the shared class is not a question for this PR! |
2 participants
To close #191