Validation vs custom properties

[jonthegeek]

My mental model is that properties are responsible for validating themselves (for the most part), but the class validator can check to make sure that the properties work with one another. However, a property with a setter breaks that model.

library(S7)
class_testing <- new_class(
  name = "testing",
  properties = list(
    first = class_character,
    second = new_property(
      setter = function(self, value) {
        # Do some things to prep value, then...
        self@second <- value
        self
      }
    )
  ),
  validator = function(self) {
    message("Validating the properties against one another")
  }
)
testing <- class_testing()
#> Validating the properties against one another
testing@second <- "this does not trigger validation"
testing@first <- "this triggers validation"
#> Validating the properties against one another

^{Created on 2023-12-05 with reprex v2.0.2}

Is this working as intended? Is there a way to trigger overall validation after a property that has a setter is updated?

[jonthegeek]

I think MAYBE I'm supposed to manually validate in the setter in cases where I want to validate. That causes an issue during initialization, but I was able to work around it. Long-term, that feels likely to cause confusion, and I'm still not sure whether it's intentional.

[hadley]

Hmmmm, not if this is a bug or not, but if it isn't we should document that you need to call validate() yourself if you're providing a custom setter.

[jonthegeek]

It's a pain during initialization, because things "after" this property aren't set yet (so validation of the overall thing definitely can fail). I worked around it by setting an "initialized" attribute if it didn't exist (and not validating), or validating if the initialized attribute is there. Definitely hacky right now but it gets the job done.

[t-kalinowski]

I think this will be fixed once https://github.com/RConsortium/S7/pull/396 is merged. The current prop<- method does not run validate(object) after running a custom setter, but it should.

[t-kalinowski]

This is fixed on main now. Thanks for reporting!