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

get_code to return concatenated code string and removed format_expression #176

Merged
merged 6 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

* Exported the `qenv` class from the package.
* The `@code` field in the `qenv` class now holds `character`, not `expression`.
* The `get_code` method returns a single concatenated string of the code.
* Removed internal `format_expression` function.

# teal.code 0.4.1

Expand Down
4 changes: 2 additions & 2 deletions R/qenv-constructor.R
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ setMethod(
"new_qenv",
signature = c(env = "environment", code = "expression"),
function(env, code) {
new_qenv(env, format_expression(code))
new_qenv(env, paste(lang2calls(code), collapse = "\n"))
}
)

Expand All @@ -51,7 +51,7 @@ setMethod(
"new_qenv",
signature = c(env = "environment", code = "language"),
function(env, code) {
new_qenv(env = env, code = format_expression(code))
new_qenv(env = env, code = paste(lang2calls(code), collapse = "\n"))
}
)

Expand Down
4 changes: 2 additions & 2 deletions R/qenv-eval_code.R
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,13 @@ setMethod("eval_code", signature = c("qenv", "character"), function(object, code
#' @rdname eval_code
#' @export
setMethod("eval_code", signature = c("qenv", "language"), function(object, code) {
eval_code(object, code = format_expression(code))
eval_code(object, code = paste(lang2calls(code), collapse = "\n"))
})

#' @rdname eval_code
#' @export
setMethod("eval_code", signature = c("qenv", "expression"), function(object, code) {
eval_code(object, code = format_expression(code))
eval_code(object, code = paste(lang2calls(code), collapse = "\n"))
})

#' @rdname eval_code
Expand Down
8 changes: 6 additions & 2 deletions R/qenv-get_code.R
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ setGeneric("get_code", function(object, deparse = TRUE, ...) {
setMethod("get_code", signature = "qenv", function(object, deparse = TRUE) {
checkmate::assert_flag(deparse)
if (deparse) {
object@code
if (length(object@code) == 0) {
object@code
} else {
paste(object@code, collapse = "\n")
}
} else {
parse(text = object@code, keep.source = TRUE)
}
Expand All @@ -41,7 +45,7 @@ setMethod("get_code", signature = "qenv.error", function(object) {
sprintf(
"%s\n\ntrace: \n %s\n",
conditionMessage(object),
paste(format_expression(object$trace), collapse = "\n ")
paste(lang2calls(object$trace), collapse = "\n ")
),
class = c("validation", "try-error", "simpleError")
)
Expand Down
4 changes: 2 additions & 2 deletions R/qenv-get_warnings.R
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ setMethod("get_warnings", signature = c("qenv"), function(object) {
if (warn == "") {
return(NULL)
}
sprintf("%swhen running code:\n%s", warn, paste(format_expression(expr), collapse = "\n"))
sprintf("%swhen running code:\n%s", warn, paste(lang2calls(expr), collapse = "\n"))
},
warn = as.list(object@warnings),
expr = as.list(as.character(object@code))
Expand All @@ -52,7 +52,7 @@ setMethod("get_warnings", signature = c("qenv"), function(object) {
sprintf(
"~~~ Warnings ~~~\n\n%s\n\n~~~ Trace ~~~\n\n%s",
paste(lines, collapse = "\n\n"),
paste(get_code(object), collapse = "\n")
get_code(object)
)
})

Expand Down
13 changes: 9 additions & 4 deletions R/qenv-replace_code.R
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,24 @@ setGeneric("replace_code", function(object, code) standardGeneric("replace_code"
#' @keywords internal
setMethod("replace_code", signature = c("qenv", "character"), function(object, code) {
masked_code <- get_code(object)
masked_code[length(masked_code)] <- code
object@code <- masked_code
code_lines <- unlist(strsplit(masked_code, "\n"))

if (!is.null(code_lines)) {
code_lines[length(code_lines)] <- code
object@code <- paste(code_lines, collapse = "\n")
}

object
})

#' @keywords internal
setMethod("replace_code", signature = c("qenv", "language"), function(object, code) {
replace_code(object, code = format_expression(code))
replace_code(object, code = paste(lang2calls(code), collapse = "\n"))
})

#' @keywords internal
setMethod("replace_code", signature = c("qenv", "expression"), function(object, code) {
replace_code(object, code = format_expression(code))
replace_code(object, code = paste(lang2calls(code), collapse = "\n"))
Comment on lines +47 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having second thoughts about removing format_expression. It's used in multiple places. I think it can stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a similar thought previously, but following a conversation with @chlebowa it was clarified that the function simply uses paste with a designated collapse parameter after processing with lang2calls. The format_expression function originally had a more complex design but it was simplified into a simple one-liner in a past refactoring phase. Currently, it's essentially redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. We don't want to have too simple wrappers. Check out the use of lapply(x, [[, y) throughout teal.slice. That used to be a function but we decided to use base calls.

On top of that, soon we will be down to just a few uses of format_expression anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format_expression sounds like a wrapper for paste, where you overwrite default collapse param :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you guys consider having lang2calls return the paste(...)?

It seems that it is only used this way in the package (other than in tests)

ps. and rename it to lang2str or lang2character?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is recursion in lang2calls and implementing the paste function with collapse directly inside lang2calls can lead to unintended results due to concatenation of results from each recursive call.

})

#' @keywords internal
Expand Down
6 changes: 0 additions & 6 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ dev_suppress <- function(x) {
force(x)
}

format_expression <- function(code) {
code <- lang2calls(code)
paste(code, collapse = "\n")
}


# convert language object or lists of language objects to list of simple calls
# @param x `language` object or a list of thereof
# @return
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ qenv_2[["y"]]
```

```r
cat(paste(get_code(qenv_2), collapse = "\n"))
cat(get_code(qenv_2))
#> x <- 5
#> y <- x * 2
#> z <- y * 2
Expand Down
4 changes: 3 additions & 1 deletion tests/testthat/test-qenv-replace_code.R
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ testthat::test_that("code_replace replaces last element of the code", {
qr <- replace_code(qq, replacement)
previous <- get_code(qq)
current <- get_code(qr)
testthat::expect_identical(current, c(head(previous, -1), replacement))

previous <- head(strsplit(previous, split = "\n")[[1]], -1)
testthat::expect_identical(current, paste0(c(previous, replacement), collapse = "\n"))
})

# edge cases ----
Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/test-qenv-within.R
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ testthat::test_that("styling of input code does not impact evaluation results",
all_code <- get_code(q)
testthat::expect_identical(
all_code,
rep("1 + 1", 4L)
paste(rep("1 + 1", 4L), collapse = "\n")
)

q <- new_qenv()
Expand All @@ -48,7 +48,7 @@ testthat::test_that("styling of input code does not impact evaluation results",
all_code <- get_code(q)
testthat::expect_identical(
all_code,
rep("1 + 1\n2 + 2", 4L)
paste(rep("1 + 1\n2 + 2", 4L), collapse = "\n")
)
})

Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/test-qenv_get_code.R
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
testthat::test_that("get_code returns code (character by default) of qenv object", {
q <- new_qenv(list2env(list(x = 1)), code = quote(x <- 1))
q <- eval_code(q, quote(y <- x))
testthat::expect_equal(get_code(q), c("x <- 1", "y <- x"))
testthat::expect_equal(get_code(q), paste(c("x <- 1", "y <- x"), collapse = "\n"))
})

testthat::test_that("get_code returns code elements being code-blocks as character(1)", {
Expand All @@ -13,7 +13,7 @@ testthat::test_that("get_code returns code elements being code-blocks as charact
z <- 5
})
)
testthat::expect_equal(get_code(q), c("x <- 1", "y <- x\nz <- 5"))
testthat::expect_equal(get_code(q), paste(c("x <- 1", "y <- x\nz <- 5"), collapse = "\n"))
})

testthat::test_that("get_code returns expression of qenv object if deparse = FALSE", {
Expand Down
44 changes: 0 additions & 44 deletions tests/testthat/test-utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -59,47 +59,3 @@ testthat::test_that("lang2calls returns atomics and symbols wrapped in list", {
testthat::expect_identical(lang2calls(list("x")), list("x"))
testthat::expect_identical(lang2calls(list(as.symbol("x"))), list(as.symbol("x")))
})


testthat::test_that(
"format_expression turns expression/calls or lists thereof into character strings without curly brackets",
{
expr1 <- expression({
i <- iris
m <- mtcars
})
expr2 <- expression(
i <- iris,
m <- mtcars
)
expr3 <- list(
expression(i <- iris),
expression(m <- mtcars)
)
cll1 <- quote({
i <- iris
m <- mtcars
})
cll2 <- list(
quote(i <- iris),
quote(m <- mtcars)
)

# function definition
fundef <- quote(
format_expression <- function(x) {
x + x
return(x)
}
)

testthat::expect_identical(format_expression(expr1), "i <- iris\nm <- mtcars")
testthat::expect_identical(format_expression(expr2), "i <- iris\nm <- mtcars")
testthat::expect_identical(format_expression(expr3), "i <- iris\nm <- mtcars")
testthat::expect_identical(format_expression(cll1), "i <- iris\nm <- mtcars")
testthat::expect_identical(format_expression(cll2), "i <- iris\nm <- mtcars")
testthat::expect_identical(
format_expression(fundef), "format_expression <- function(x) {\n x + x\n return(x)\n}"
)
}
)
2 changes: 1 addition & 1 deletion vignettes/qenv.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ To extract objects from a `qenv`, use `[[`; this is particularly useful for disp
```{r}
print(q2[["y"]])

cat(paste(get_code(q2), collapse = "\n"))
cat(get_code(q2))
```

### Substitutions
Expand Down