Teal refactor

[pawelru]

Part of insightsengineering/teal#731

[pawelru]

I am a little bit concerned here about get() operation as it could evaluate lazy bindings or promises therefore this functionality could be expensive and most importantly - it could have some side effects. I neither have a counter-example nor a potential solution. Just note to myself...

[pawelru]

it's always good to assure that function always returns the same data type. This would prevent you doing expect_true(foo(...)) and expect_match(foo(...), "...") at the same time.

This is not the first time where we are facing the problem of returning a character message if something else TRUE. I suggest to always return boolean with additional attribute. Alternatively you might want to use checkmate assertion collections.

[asbates]

I agree. As a user it can be annoying when a function returns 2 types of objects. ('user' includes developer w/in package)

[gogonzo]

This function is internal, user shouldn't use this function at all. .check_joinable is used only in the join in following way:

  join_validation <- .check_joinable(x, y)

  # join expressions
  if (!isTRUE(join_validation)) {
    stop(join_validation)
  }

Alternatively, we can move stop to the .check_joinable and forget the whole thing

[nikolas-burkoff]

See #88 (for later)

[nikolas-burkoff]

Just confirming this test leaves the status of the checkmate package unchanged (we don't need a on.exit(detach("package:checkmate")) on line 29 to make sure that it is detached if something goes wrong in lines 32 and 33? @gogonzo

[nikolas-burkoff]

Lets do this 🦾

[pawelru]

can we remove cli dependency? It's a big package

[nikolas-burkoff]

if we do then we don't fix this:
#86 - unless you want to fix it a different way

[pawelru]

that's my question - can we do (name that thing) differently not involving cli package?

[nikolas-burkoff]

so this is what we call:

> cli::ansi_strip
function (string, sgr = TRUE, csi = TRUE, link = TRUE) 
{
    if (!is.character(string)) 
        string <- as.character(string)
    string <- enc2utf8(string)
    stopifnot(is_flag(sgr), is_flag(csi), is_flag(link))
    clean <- .Call(clic_ansi_strip, string, sgr, csi, link)
    class(clean) <- setdiff(class(clean), c("cli_ansi_string", 
        "ansi_string"))
    clean
}
<bytecode: 0x0000018dd338c3e0>
<environment: namespace:cli>

[nikolas-burkoff]

and dplyr imports cli so in practice ppl using this package will have cli installed but if there's an easy way round this then happy to change (or put issue in bnacklog to change)

[nikolas-burkoff]

we could add it into suggests like rlang does and only call it if installed?

[pawelru]

Here I am more concerned about the package size and the installation process. Up to you how would you like to process it.

[pawelru]

we could add it into suggests like rlang and only call it if installed?

This only makes sense if a particular piece of code is unlikely to be called assuming standard workflow. E.g. a wide tree-based workflow / case when where there are many many possibilities. Here, if I understand everything correctly it's a part of standard logic so it's very likely that it will be executed.

[nikolas-burkoff]

so I assume that these funny terminal printing characters added by rlang are only added if cli is installed (cli is a suggests in rlang only) - therefore I think it makes sense for us to use suggests as well - as striping these terminal printing characters out only makes sense if cli (via rlang) added them in the first place - commit coming soon

[nikolas-burkoff]

so rlang is only in the suggests but needs to be imported - whoops

[pawelru]

And we are one step away from test coverage perfection :). In the covr report there is only one line non-covered in tests:

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------
R/qenv-eval_code.R          45       1  97.78%   89

[gogonzo]

For anyone concerned about deep coping the objects. There are no deep copies in eval_code and join, we only create a new environment each time, set parent to .globalEnv and we create a new reference to the same pointer in the new env.

library(teal.code)
library(lobstr)
x <- data.frame(a = runif(10))

q1 <- new_qenv(env = list2env(list(x = x)), code = "x <- data.frame(a = 1)")

identical(lobstr::ref(q1[["x"]]), lobstr::ref(x))
# TRUE

q2 <- eval_code(q1, quote(x$b <- runif(10)))

!identical(lobstr::ref(q2[["x"]]), lobstr::ref(q1[["x"]]))
# TRUE
identical(lobstr::ref(q2[["x"]]$a), lobstr::ref(q1[["x"]]$a))
# TRUE

q3 <- eval_code(q2, quote(y <- x))
identical(q3[["y"]], q2[["x"]])
# TRUE

q4 <- join(q2, q3)
identical(lobstr::ref(q4[["y"]]), lobstr::ref(q2[["x"]]))
# TRUE

[donyunardi]

qenv vignette was clear and easy to follow. Great job!