Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink assertion factories API #39

Open
moodymudskipper opened this issue Sep 27, 2023 · 1 comment
Open

Rethink assertion factories API #39

moodymudskipper opened this issue Sep 27, 2023 · 1 comment
Milestone

Comments

@moodymudskipper
Copy link
Owner

We have several issues :

  • fun = output args can't be used with pkg::fun
  • our default names conflict with function, e.g. we have a special arg for length, that does not map directly to the length function
  • anyDuplicated = 0L would work but anyDuplicated = 0 wouldn't
  • feeding unnamed args to the ... is awkward, skipping args is weird, ... = is weirder
  • the formula syntax for custom errors is weird.

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
Copy link
Owner Author

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))
}

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

No branches or pull requests

1 participant