Skip to content

Create S3 generic return_()#133

Closed
Bisaloo wants to merge 3 commits intomasterfrom
return-S3
Closed

Create S3 generic return_()#133
Bisaloo wants to merge 3 commits intomasterfrom
return-S3

Conversation

@Bisaloo
Copy link
Copy Markdown
Contributor

@Bisaloo Bisaloo commented Aug 26, 2021

As discussed in #130.

Feel free to edit anything in any way you like before merging.

@Bisaloo Bisaloo requested a review from nikosbosse August 26, 2021 14:30
@Bisaloo
Copy link
Copy Markdown
Contributor Author

Bisaloo commented Aug 26, 2021

Maybe it should not be exported 🤔

@nikosbosse
Copy link
Copy Markdown
Collaborator

Thanks a lot, this is really cool! 2 questions / comments

  1. Pure learning-question: I haven't used S3-methods myself yet. Are they superior in some sense? Or would it be the same to have a function
return_ <- function(x) {
  if (data.table %in% class(x)) {
    return(x[])
  } else {
    return(x)
  }
}
  1. There is an issue with the code as is: I think right now return_ doesn't cause function execution to end immediately. I think tests fail because sometimes, if there is a return statement in a function rather than at the end it will still execute the remainder of the code and then return NULL. Is there a way to fix this?

@Bisaloo
Copy link
Copy Markdown
Contributor Author

Bisaloo commented Aug 26, 2021

Pure learning-question: I haven't used S3-methods myself yet. Are they superior in some sense? Or would it be the same to have a function

For the case at hand, I'd say it's equivalent. The main benefits (as far as I know) of using S3 in the general case:

  • extensibility: once use defined the generic return_(), anybody can define their own method for their custom object. A good example for this is knitr::knit_print(). data.table, tibble, or really any other package can define how the objects they are creating will be printed in a Rmd document. And as you see with this example, methods can be defined in a different package than the generic, that's not an issue.
  • automatic dispatch based on order of elements in the class attribute. Let's complexify a little the example code you posted in the previous comment with another method (on an unrelated note, it's better to use inherits() to test class inheritance):
f <- function(x) {
  if (inherits(x, "class1") {
    do1(x)
  } else if (inherits(x, "class2") {
    do2(x)
  } else {
    do3(x)
  }
}

Now what happens when you have an object x1 with class c("class1", "class2")? Since you start your if/else chain with class1, you would get action do1. And what about x2 with c("class2", "class1")? Well, exactly the same.

If we use a S3 system in this case, x1 will trigger do1 and x2 will trigger do2 because it's based on which element appears first. (Note that if we only define a method for class1, both x1 and x2 will trigger do1.)

@Bisaloo
Copy link
Copy Markdown
Contributor Author

Bisaloo commented Aug 26, 2021

For the other point, I'll think about it. It might be the thing that I overlooked and that makes this impossible (hence why nobody suggested this idea).

@nikosbosse
Copy link
Copy Markdown
Collaborator

I'm closing this for now as we're not sure what to do with the return statements. Please feel free to open again!

@nikosbosse nikosbosse closed this Nov 21, 2021
@nikosbosse nikosbosse deleted the return-S3 branch November 25, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants