[Feature Request]: should `verify()` return a object?

[pawelru]

Feature description

I have a problem with understanding the following:

data <- ...
data <- verify(data)
...

For me, the verify() should be more similar to the assert like below:

data <- ...
verify(data)
...

With this, we should probably modify verify() so that it does not return if there is no error.

Ideally, we should have assert() that raises error or silently pass and check() that returns boolean. I just can't fit the current implementation of verify() that throws or pass the object. WDYT?

Code of Conduct

• I agree to follow this project's Code of Conduct.

Contribution Guidelines

• I agree to follow this project's Contribution Guidelines.

Security Policy

• I agree to follow this project's Security Policy.

[donyunardi]

As a reference, here are the issue/PR when we created the slot/method:

• [Feature request] Introduce validate method for teal_data #182
• 182 introduce @verified slot in teal_data class #188

We want the verify() method to update the @verified slot in teal_data object and we preferred the method to return a new object as this slot will also be used in teal modules. If @verified is not TRUE, I believe we have put some mechanics to alert the user in the teal module. So I see the value of having this slot for downstream consumers. I also think it's nice to return a verified teal_data object to give that boost of confidence to user on one of a highlighted feature of our product, code reproducibility.

Worth to mention that verify() method only make sense if user is using the teal_data(code=) argument.

With within, we will check if the code is reproducible and set the @verified slot accordingly.

teal.data/R/verify.R

Lines 70 to 74 in e88539f

	reproduced <- isTRUE(all.equal(teal.code::get_env(x), teal.code::get_env(y)))
	if (reproduced) {
	x@verified <- TRUE
	methods::validObject(x)
	x

I just can't fit the current implementation of verify() that throws or pass the object.

If I'm a teal app programmer using teal_data(code=), I think I would prefer to know that my data is faulty early on rather than later when the teal module process my data.

I'm open on having an assertion but we should do it in addition of the verify(), not replacement so developer has option.
If we want to remove verify(), then we should think on what to do with teal_data(code=) argument.

This is my initial thought, hopefully it's enough to start a discussion.

[donyunardi]

If @verified is not TRUE, I believe we have put some mechanics to alert the user in the teal module.

I need to correct this statement. It looks like we're not showing any message in the module if teal_data is not verified.

We're also no longer showing any message in show R code.
#334

The result is that, I am still able to run teal app with unverified teal_data and show R Code will show code that is not reproducible.

library(teal)

data <- teal_data(
  iris = iris,
  code = "
    iris2 <- iris
  "
)

app <- init(
  data = data,
  modules = modules(
    example_module(label = "my module")
  )
)

shinyApp(app$ui, app$server)

I know we're pushing for user to use within but we should still take into account that user still have the ability to use teal_data().

[pawelru]

Thanks for this. It's getting more and more clear. I totally get why this method is needed - it's just not very clear what it returns and why.

This method is for an object of R6 class which is mutable by its definition. That means that you don't have to overwrite the instance on setters. You usually do: x$set_value(T) as opposed to x <- x$set_value(T). In R, this can also mean to return an object invisibly to enable chaining x$set_a(1)$set_b(2)$... but this is optional/extra I would say.

The problem is that this method is not a pure setter. It actually executes something and as a result set up (internal?) flag. I would say this is more of a action method instead. These family of method usually does not return the instance.

Let me give you a dummy example: x <- liner_regression_object(...). Let's assume that there is a calculate method that writes internally to its fields. In order to run the calculation you would do x$calculate() (as opposed to x <- x$calculate()). And the method should return some sort of summary of calculation so it's rather result <- x$calculate().

Similarly here - IMHO verify() should return the verification outcome if one is interested - not the whole object.

[donyunardi]

teal_data is now an S4 object which is immutable by convention.

I don't believe we can do x$verify() in S4 which is why we have to create generic function verify() to dispatch the method. And by design, S4 will return a modified copy of the object.