-
-
Notifications
You must be signed in to change notification settings - Fork 8
Keeps last value of eval_code()/within()
#257
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
Conversation
Code balance is + because I've added one test ;]
Companion to insightsengineering/teal.reporter#334 Consequence of changing naming convention for `teal_report` object.
|
Should we add a getter function? |
R/qenv-eval_code.R
Outdated
| { | ||
| eval(current_call, envir = object@.xData) | ||
| .Last.value <- eval(current_call, envir = object@.xData) | ||
| attr(object@.xData, ".Last.value") <- .Last.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing to base - .Last.value isn't an attribute but an object of the .GlobalEnv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, even worse. .Last.value is an object in base and not assigned to any environment :/
a <- "a"
c <- "c"
a
base::.Last.value
# [1] "a"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we cannot fully reproduce it, it would just follow the concept.
…table
* origin/main:
[skip actions] Bump version to 0.6.1.9003
`{teal}` module returns a `teal_report` object that extends from `teal_data` (#255)
Unit Tests Summary 1 files 13 suites 4s ⏱️ Results for commit d6c9c0d. ♻️ This comment has been updated with latest results. |
Unit Test Performance DifferenceAdditional test case details
Results for commit bef1907 ♻️ This comment has been updated with latest results. |
Code Coverage SummaryDiff against mainResults for commit: d6c9c0d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
gogonzo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Attribute of the environment" is not exactly what it is
| ) | ||
| }) | ||
|
|
||
| testthat::test_that("eval_code keeps .Last.value as an attribute of the environment", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| testthat::test_that("eval_code keeps .Last.value as an attribute of the environment", { | |
| testthat::test_that("eval_code stores .Last.value in the parent environment", { |
| testthat::expect_identical(get_code(q), "a <- 1L") | ||
| }) | ||
|
|
||
| testthat::test_that("within keeps .Last.value as an attribute of the environment", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| testthat::test_that("within keeps .Last.value as an attribute of the environment", { | |
| testthat::test_that("within eval_code stores .Last.value in the parent environment", { |
| testthat::test_that("within keeps .Last.value as an attribute of the environment", { | ||
| q <- within(qenv(), x <- 1) | ||
| env <- parent.env(q) | ||
| testthat::expect_true(".Last.value" %in% names(env)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not needed as the next one confirms this also
| testthat::expect_true(".Last.value" %in% names(env)) |
| testthat::test_that("eval_code keeps .Last.value as an attribute of the environment", { | ||
| q <- eval_code(qenv(), quote(x <- 1)) | ||
| env <- parent.env(q) | ||
| testthat::expect_true(".Last.value" %in% names(env)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not needed as the next one confirms this also
| testthat::expect_true(".Last.value" %in% names(env)) |
|
|
||
| ### Enhancements | ||
|
|
||
| * Code evaluation keeps the last evaluated expression in the `.Last.value` attribute of the environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Code evaluation keeps the last evaluated expression in the `.Last.value` attribute of the environment. | |
| * Code evaluation keeps the last evaluated expression in the `parent.env(<qenv>)$.Last.value`. |
gogonzo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the best proposition so far. I don't think we will be able or it will be worth it to follow the rmarkdown concept of fetching everything like it was called line by line in the interactive session:
I think it is a good compromise easy to cache always the .Last.value in qenv. We will work on the teal.reporter followup implementation of this 👍
|
@gogonzo Should we pause/discard this PR given your new discovery late yesterday and our call today in favor of using This could live as its own feature though. |
|
Closed in favor of #259 |
New feature for
teal.codethat keeps the last value of the code evaluation.In line with R's
.Last.value(see doc)Cloning
Created on 2025-06-12 with reprex v2.1.1