Convenience props

[lawremi]

@t-kalinowski here are the convenience property constructors that we discussed. They seem useful in my applications of S7; however, that does not necessarily mean that they belong in the base S7 package. Curious about everyone's thoughts.

[hadley]

Sorry, I missed this! I've been thinking about this too for elmer, whose classes have almost entirely scalar properties. I do think that this does belong in S7 because scalar values are such a common need and we don't currently offer any help.

My model for thinking about this is based on the check_* functions that we've implementing in rlang, and embed into most tidyverse packages: https://github.com/r-lib/rlang/blob/main/R/standalone-types-check.R. We use them a lot so they're pretty battle tested.

For this discussion, I think the most relevant are:

• check_bool(), equivalent to your new_flag_property()
• check_string(), equivalent to your new_string_property().
• check_number_decimal(), equivalent to your new_number_property().
• check_number_whole(), equivalent to your new_int_property().

A few thoughts based on comparing our code to your properties:

• We should negotiate on the names to get something we're both happy with 😄

• All our checks have an allow_na = FALSE and allow_null = FALSE argument. These turn out to be pretty useful defaults since most of the time (at least in our experience) scalars shouldn't be missing, and it's nice to have an easy way to make them optional. We also have allow_empty for check_string(), and that comes up quite frequently.

• I don't think scalar properties should have a default (unless they're nullable/optional). There's an obvious default for a vector (a zero length instance), but I don't think that's the case for scalars.

• We spent a bunch of time to come up with _number_whole() vs _number_decimal() and I like how they focus on the type of number rather than the type of the underlying storage. As you point out, forcing users to supply literal integers is rather hostile.

• I'd be tempted to give these property constructors a scalar_ prefix, because I think that's a nice constrast with the typical class_ prefix, e.g.

  new_class("foo", properties = list(
    x = class_numeric,
    open = scalar_bool(),
    times = scalar_number_whole()
  ))

• Maybe it's worth having an explicit scalar_enum() property? That would correspond to match.arg() or rlang::arg_match() (which is similar but doesn't do partial matching and includes an error message that uses string distance to suggest which option you might have wanted.)

• I think we should discuss the list checker in more detail because I think it's useful to also be able to provide the specific names you expect. But then you need some way to say if any additional names are ok too.

• I do wonder if it's worth including something for paths, since that's also a common need.

Thanks for working on this and I look forward to discussing it more 😄

[lawremi]

Thanks for your comments. They all seem reasonable to me. I don't feel that strongly about the details, as long as the convenience and code clarity remains. The only reason I messed around with default defaults is that they weren't hurting anything, but I agree it might be better to force the developer to be explicit.

[t-kalinowski]

It would also be nice to allow for convenient setter actions associated with properties. In particular:

• make it easy to create a coercing setter, which will call convert() on the value provided.
• make it easy to create a "set-once" setter, which will error if the underlying attr(self, "name") is not NULL.

A sketch:

new_setter <- function(set_once = FALSE, coerce = TRUE, nullable = TRUE) {
  set_once; coerce

  if (isTRUE(coerce)) {
    coerce <- function(value) {
      class <- eval(quote(S7_class(self)@properties[[name]]$class),
                    envir = parent.frame())
      if(nullable)
        for(class in class$classes)
          if(!is.null(class))
            break

      convert(value, class)
    }
  } else if (isFALSE(coerce) || is.null(coerce)) {
    coerce <- identity
  } else {
    stopifnot(is.function(coerce))
  }

  function(self, value) {
    name <- attr(self, ".setting_prop", TRUE) |> tail(1) |> as.character()
    if (set_once) {
      if (!is.null(prop(self, name)))
        stop(name, " can only be set once.")
    }

    if(nullable && is.null(value))
      return(self)

    prop(self, name) <- coerce(value)
    self
  }
}


prop_string <- function(default = NULL,
                        allow_null = FALSE,
                        allow_na = FALSE,
                        setter = NULL) {
  force(allow_null)
  force(allow_na)

  new_property(
    class = if (allow_null) NULL | class_character else class_character,
    default = default,
    setter = setter,
    validator = function(value) {
      if (allow_null && is.null(value)) {
        return()
      }

      if (length(value) != 1) {
        paste0("must be a single string, not '", as.character(value), "'.")
      } else if (!allow_na && is.na(value)) {
        "must not be missing."
      }
    }
  )
}

used like:

Foo := new_class(
  properties = list(
    name = prop_string(
      allow_null = FALSE,
      default = quote(stop("name must be provided")),
      setter = new_setter(set_once = TRUE, nullable = FALSE)
    ),
    read_only = prop_bool(allow_null = TRUE, 
                          setter = new_setter(set_once = TRUE))
  ))

[lawremi]

Interesting ideas. We might consider those behaviors the domain of the property object itself, leaving the setter to be defined as normal. Any user setter would get wrapped with the extra logic.

[t-kalinowski]

Some motivators for automatically calling convert() from @<-:

• for a property of class_character, I want to be able to supply NA and not NA_character_.
• for a property of class_integer, I want to be able to supply a whole-number double (or NA).

[hadley]

I'm sceptical that we want to automatically convert, because I think automatic coercions tend to hide problems that you want to know about. I think scalar property helpers will need something custom, because if you have an integer property, I think that 2 is an acceptable value but 2.5 should throw an error.