127 with(in) qenv

[chlebowa]

Closes #127

CURRENT IMPLEMENTATION

(NOTE: the new class is called quenv in the code to avoid conflicts with the existing qenv).

qenv is a simple S3 class, an environment with some attributes: code, errors, warnings, messages.
with.qenv method is used to evaluate code in the qenv.

Syntax

Just pass an expression to the expr argument in with call.
Simple expressions should be enclosed in braces to store the code properly. Non-braced expressions are handled for convenience but it may be safer to use braces at all times.

q <- qenv()
with(q, {
  i <- iris
})
with(q, i <- iris)                 # alternative syntax

Compound expressions are supported but consecutive lines are evaluated separately, in turn.

with(q, {
  m <- mtcars
  ms <- subset(m, cyl == 4)
})

If a simple expression that is part of a compound expression raises an error, the following lines are not evaluated.

with(q, {
  w <- warpbreaks
  stop("that's far enough")        # stops evaluation
  w <- subset(w, wool == "B")     # not evaluated
})

Code can also be passed as strings, both literals and values, through the text argument.

xx <- "cc <- cars"
with(q, text = "cc <- cars")
with(q, text = xx)                 # equivalent to the above

Strings passed to the expr raise errors. Strings passed as parts of compound expressions are ignored.

with(q, "print(dim(m))")           # raises error
with(q, {"print(dim(m))"})         # not evaluated
with(q, text = "print(dim(m))")    # evaluated
with(q, print(dim(m)))             # equivalent to the above

The text argument can be used to evaluate code imported from a file.

with(q, text = readLines("code_file"))

Evaluation

Expression are evaluated in the qenv (per definition of the with generic it is called data) and the enclosing environment is the default parent.frame(). This includes the global environment in the scope, which makes the i <- iris calls possible.

In order to use expressions obtained programmatically, i.e. pass values that are not defined neither in data, nor in the parent frame, the expressions are first substituted in the current environment, i.e. the execution environment of the with call. Thus, we simply pass any extra values to the ellipsis:

q <- qenv()
with(
  data = q,
  expr = {
    i <- iris
    ii <- subset(i, Species == species)
  },
  species = "versicolor"
)

The same mechanism can utilize variables defined outside of the with call.

input_value <- "virginica"
q <- qenv()
with(
  data = q,
  expr = {
    i <- iris
    ii <- subset(i, Species == species)
  },
  species = input_value
)

Tracking qenv history

Each (single line) expression is stored in the code attribute, which is a list of expressions. After running the first code block, attr(q, "code") will be a list containing two expressions, both of them i <- iris.

The expression that is stored is the expression that is evaluated, after substitution of external values is complete. In the last two examples the subset expressions would be stored as ii <- subset(i, Species == "versicolor") and ii <- subset(i, Species == "virginica"), NOT as ii <- subset(i, Species == species) and ii <- subset(i, Species == input_value), respectively.

Storing conditions

After an expression has been evaluated, messages from any conditions raised are stored in respective attributes, which are lists of character strings.
If no condition is raised, an empty string is stored so that each condition list has the same length as code and condition messages can be easily matched to the code that raised them.

Utilities.

An elaborate format method prints a summary of contents, all expressions evaluated and all condition messages.
get_code and get_condition functions are provided for convenient retrieval of attributes.
Values are retrieved with the usual indexing operators, $ and [[.

within

within.qenv works exactly like with.qenv, except it creates a deep copy of the qenv and returns the result.

q <- qenv()                        # create qenv
with(qenv, i <- iris)              # modify qenv
qq <- within(q, m <- mtcars)       # copy qenv, then modify and return the copy

[averissimo]

Simple expressions should be enclosed in braces to store the code properly. Non-braced expressions are handled for convenience but it may be safer to use braces at all times.

Have you considered showing a message or warning every 8 hours to discourage this behaviour?

Suggested change

	expr <- call("{", expr)
	rlang::inform(
	"Please refrain from using expressions that are not surrounded by braces (use `{ ... }` even for a single line expression)",
	.frequency = "regularly",
	.frequency_id = "curly_expr_info"
	)
	expr <- call("{", expr)

[chlebowa]

I hate rlang 😉

No, I haven't. Thanks for the suggestion.

[averissimo]

This looks really good!! 👍

I played around with it and found some (possible) issues with text expressions.

• [text param] Single expression spanning more than 1 line
• [text param] Compound expression
• [text param] Ends with newline

Regarding the first 2 I commented on the code, the last one is just a minor detail 😅

All 3 problems seem to go away when using str2expression, but I'm guessing there's a reason not to use that function

testthat::test_that("with.qenv Supports text expression that snds with a newline", {

  str_expr <- "1 + 1
  2 * 3
"

  script_path <- withr::local_file(tempfile())
  writeLines(str_expr, script_path)

  q <- qenv()

  with(q, text = readLines(script_path)) |>
    testthat::expect_no_error()
})

[chlebowa]

Thanks @averissimo! Since this is not a priority now I wasn't expecting a proper review yet so I'm happy to see some interest :)
Good work finding more edge cases, I'll have a look.
I did try going with str2expression but there was some reason against it which I now forget. I'll give it another try.

[chlebowa]

Right, the reason I decided against str2expression is I wanted each line of compound expressions to be evaluated separately. That way if an error occurs, we can pinpoint the exact problem and display it to the user.
Theoretically this is also possible with str2expression as a compound expression can be disassembled but it is a pain to be able to handle simple things like "1 + 1".

Perhaps I missed something. Where exactly did you use str2expression?

[averissimo]

@chlebowa Sorry if I jumped the gun with the review.. this looks really good and I couldn't help myself to test it a bit ☺️

I changed the .prepare_code() function by calling str2expression() on the text param.

ℹ️ See the comment on the code with a crude proposal that may need some polishing

... I wanted each line of compound expressions to be evaluated separately

I think we can we actually run each expression separately, see the toy example below:

text1 <- "1 + 2
3*3
a <- 2 +
3
a * 5"

text2 <- "{9 + 2
8*3
c <- 2 +
4
c * 10}"

cat_expr <- \(text) {
  expr <- str2expression(text)
  
  if (length(expr) == 0) return(invisible(NULL))
  
  if (length(expr) == 1 && isTRUE(expr[[1]][[1]] == "{")) {
    expr <- expr[[1]][seq(2, length(expr[[1]]))]
  }
  
  if (length(expr) > 1) {
    expr |> 
      purrr::walk(\(.x) cat("Expression:", deparse1(.x), "\n"))
  } else {
    # Otherwise treat it as single expression
    cat("Expression:", deparse1(expr[[1]]), "\n")
  }
  invisible(NULL)
}


cat_expr(text1)
cat_expr(text2)
cat_expr("1+1")
cat_expr("")
cat_expr("NULL")
cat_expr("NA")

[chlebowa]

@chlebowa Sorry if I jumped the gun with the review.. this looks really good and I couldn't help myself to test it a bit ☺️

By all means, be my guest 🙂

Ok, I see now that if the else block in the code <- block is replaced with

      text <- str2expression(text)
      if (length(text) == 1L && isTRUE(text[[1L]][[1L]] == "{")) {
        text <- text[[1L]][-1L]
      }
      as.character(text)

then the

if (!is.null(text) && length(text) == 1L && grepl("^\\{", text)) {
    text <- strsplit(text, split = "\n")[[1]]
    text <- trimws(text, whitespace = "[ \t\r\n\\{\\}]")
    text <- Filter(Negate(function(x) identical(x, "")), text)
  }

above can be dropped.

Well done 👍

One question though: do we want to tolerate such cases (i.e. strings containing invalid expressions) or should we forbid them?

[github-actions]

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  --------------------------
R/include_css_js.R           7       7  0.00%    12-20
R/qenv-concat.R             10       0  100.00%
R/qenv-constructor.R        12       0  100.00%
R/qenv-eval_code.R          48       2  95.83%   89, 98
R/qenv-get_code.R           18       0  100.00%
R/qenv-get_var.R            19       0  100.00%
R/qenv-get_warnings.R       24       0  100.00%
R/qenv-join.R               46       0  100.00%
R/qenv-show.R                1       1  0.00%    16
R/utils.R                   19       0  100.00%
R/with-qenv.R              170      76  55.29%   123-213, 293, 359, 373-409
TOTAL                      374      86  77.01%

Diff against main

Filename         Stmts    Miss  Cover
-------------  -------  ------  -------
R/with-qenv.R     +170     +76  +55.29%
TOTAL             +170     +76  -18.09%

Results for commit: d913882

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

    1 files      9 suites   1s ⏱️
  89 tests   89 ✔️ 0 💤 0 ❌
205 runs  205 ✔️ 0 💤 0 ❌

Results for commit 0ebc5e7.

♻️ This comment has been updated with latest results.

[chlebowa]

So, properly parsing all kinds of expressions (that came to mind) took some more effort but I think i got it right now.

The idea is compound expressions are always disassembled into simple ones and those are evaluated sequentially, so that evaluation stops when an error is encountered.

On top of that, all simple expressions are stored (and printed) in the same neat form, regardless of how they were passed (braces, semicolons, line breaks, etc.).

[gogonzo]

1. I like the idea with qenv being an S3 environment.
2. This with.qenv exposes us to some risks.
3. Beware that qenv will be inherited in teal.data. Is it safe to use S3 in the way class(x) <- c("tdata", "qenv", "environment") and just register new methods for tdata?

[gogonzo]

I don't like with.quenv method, it is not compatible with generic. with.default returns result of the evaluation and here with.quenv modifies environment of the object. It's dangerous in our case where we pass object from one reactive to another - it's vulnerable that latter reactive might change object present in previous reactives (we risk double evaluation).

[gogonzo]

Suggested change

	within.quenv <- function(data, expr, text, ...) {
	within.quenv <- function(data, expr, text, ...) {

expr, text and ... in a separate fields to avoid evaluation for sake of dispatch? I think this is reasonable alternative to have multiple arguments instead of dispatch.

[gogonzo]

Suggested change

	quenv <- function() {
	ans <- new.env()
	attr(ans, "code") <- list()
	attr(ans, "errors") <- list()
	attr(ans, "warnings") <- list()
	attr(ans, "messages") <- list()
	class(ans) <- c("quenv", class(ans))
	ans
	}
	quenv <- function() {
	ans <- new.env()
	attr(ans, "code") <- list()
	attr(ans, "errors") <- list()
	attr(ans, "warnings") <- list()
	attr(ans, "messages") <- list()
	class(ans) <- c("quenv", class(ans))
	ans
	}

Smart move with qenv being an environment. Not sure yet what is better:

1. qenv as S3 environment and protecting class by overwriting $<-.qenv, [[<-.qenv but potentially also other methods (which? eval?)
2. qenv as S4 and writing own $, [[ methods to get objects.

I like qenv as environment, I think inheritance and reusing the class in other packages should be the key for a decision.

[gogonzo]

instead of $<-.quenv, [[<-.qenv it is possible to just lockEnvironment and unlock only inside within

[gogonzo]

To introduce within method we don't need to refactor whole package. We can add within.qenv <- function(data, expr, text, ...) function and decide some day about form of qenv.

[donyunardi]

Closing in favor of #149