moodymudskipper / typed

Support Types for Variables, Arguments, and Return Values
158 stars 5 forks source link

Rethink assertion factories API #39

Open moodymudskipper opened 9 months ago

moodymudskipper commented 9 months ago

We have several issues :

All these flows make the package awkward to use. I think I tried too hard to make the syntax compact so we could define types inline, but with #35 we can provide guidelines to make it easy.

It would be great if we could define a type inheriting from another type, without using as_assertion, and still define arguments named as we want, and errors as we want. It doesn't matter if the DEFINITION of the type is verbose, but it should not be confusing, only the USAGE of the type should be pretty and compact.

moodymudskipper commented 7 months ago

I prototyped this (not pushed), using a .extend arg.

It's not a lot of code to make this work but I wonder if I should do a full pass on the package to improve consistency, use rlang errors and try_fetch, and use .extend to define everything.

On the way we keep backward compat with the weird idioms we had but we convert them to the new stuff and we don't document them anymore.

I think a general issue this package has is that I wanted to inline code too much, so we could print functions and understand the logic and checks more easily. I think it worked out poorly and created these environment issues.

I think a better design would be to have all assertions factories with a same code, but a different proper enclosure, no inlining of values, and we can have a helper to fetch assertions. inheritance is just adding assertions to the list.

Data.frame2 <- function(..., names_include = NULL, names_among = NULL, ptype = NULL) {

  check_names_include <- function(x) {
    if (is.null(names_include)) return(NULL)
    if(!all(names_include %in% names(x))) {
      msg <- "Wrong column names"
      info1 <- sprintf("Expected to include: %s", toString(names_include))
      info2 <- sprintf("Got : %s", toString(names(x)))
     rlang::abort(
       c(msg, i = info1, x = info2)
     ) 
    }
  }

  check_names_among <- function(x) {
    if (is.null(names_among)) return(NULL)
    if(!all(names(x) %in% names_among)) {
      msg <- "Wrong column names"
      info1 <- sprintf("Expected to be a subset of: %s", toString(names_among))
      info2 <- sprintf("Got : %s", toString(names(x)))
     rlang::abort(
       c(msg, i = info1, x = info2)
     ) 
    }
  }

  check_ptype <- function(x) {
    if (is.null(ptype)) return(NULL)
    if(!all(names(x) %in% names_among)) {
      vctrs::vec_assert(x, ptype)
    }
  }

  Data.frame(..., .extend = list(check_names_include, check_names_among, check_ptype))
}