Skip to content

Keeps last value of eval_code()/within()#257

Closed
averissimo wants to merge 19 commits into
mainfrom
last_value@teal_reportable
Closed

Keeps last value of eval_code()/within()#257
averissimo wants to merge 19 commits into
mainfrom
last_value@teal_reportable

Conversation

@averissimo

@averissimo averissimo commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

New feature for teal.code that keeps the last value of the code evaluation.

In line with R's .Last.value (see doc)

pkgload::load_all("../teal.code")
#> ℹ Loading teal.code

q <- qenv() |> within({
  1+1
}) |> within({
  aa <- 2
})
q
#> <environment: 0x616d80da6ec0> 🔒 
#> Parent: <environment: devtools_shims> 
#> Bindings:
#> - aa: [numeric]

q@.xData |> attributes()
#> $.Last.value
#> [1] 2

library(dplyr) |> suppressPackageStartupMessages()

q2 <- q |> within(
  bb <- 3
)
q2
#> <environment: 0x616d80725698> 🔒 
#> Parent: <environment: package:dplyr> 
#> Bindings:
#> - aa: [numeric]
#> - bb: [numeric]
q2@.xData |> attributes()
#> $.Last.value
#> [1] 3

Cloning

rlang::env_clone(as.environment(q2)) |> attributes()
#> NULL

Created on 2025-06-12 with reprex v2.1.1

image

@averissimo
averissimo requested a review from gogonzo June 12, 2025 10:18
@averissimo

Copy link
Copy Markdown
Contributor Author

Should we add a getter function?

Comment thread 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

Copy link
Copy Markdown
Contributor

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

@gogonzo gogonzo Jun 12, 2025

Copy link
Copy Markdown
Contributor

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"

Copy link
Copy Markdown
Contributor Author

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.

Base automatically changed from teal_reportable to main June 12, 2025 14:44
…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)
@github-actions

github-actions Bot commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

Unit Tests Summary

  1 files   13 suites   4s ⏱️
159 tests 156 ✅ 3 💤 0 ❌
249 runs  246 ✅ 3 💤 0 ❌

Results for commit d6c9c0d.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_eval_code 👶 $+0.02$ eval_code_keeps_.Last.value_as_an_attribute_of_the_environment
qenv_within 👶 $+0.02$ within_keeps_.Last.value_as_an_attribute_of_the_environment

Results for commit bef1907

♻️ This comment has been updated with latest results.

@github-actions

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ---------
R/qenv-c.R                          55       0  100.00%
R/qenv-class.R                      12       0  100.00%
R/qenv-concat.R                      7       0  100.00%
R/qenv-constructor.R                 1       0  100.00%
R/qenv-errors.R                      4       4  0.00%    6-9
R/qenv-eval_code.R                  62       1  98.39%   42
R/qenv-extract.R                    30       0  100.00%
R/qenv-get_code.R                   24       0  100.00%
R/qenv-get_env.R                     3       1  66.67%   27
R/qenv-get_messages.r                5       0  100.00%
R/qenv-get_var.R                    26       0  100.00%
R/qenv-get_warnings.R                5       0  100.00%
R/qenv-join.R                        7       7  0.00%    137-151
R/qenv-length.R                      2       1  50.00%   2
R/qenv-show.R                       29      29  0.00%    19-50
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      245       2  99.18%   160, 258
R/utils.R                           30       0  100.00%
TOTAL                              555      45  91.89%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: d6c9c0d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@gogonzo gogonzo left a comment

Copy link
Copy Markdown
Contributor

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", {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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", {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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))

Copy link
Copy Markdown
Contributor

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

Suggested change
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))

Copy link
Copy Markdown
Contributor

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

Suggested change
testthat::expect_true(".Last.value" %in% names(env))

Comment thread NEWS.md

### Enhancements

* Code evaluation keeps the last evaluated expression in the `.Last.value` attribute of the environment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 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 gogonzo left a comment

Copy link
Copy Markdown
Contributor

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:

image

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 👍

@averissimo

Copy link
Copy Markdown
Contributor Author

@gogonzo Should we pause/discard this PR given your new discovery late yesterday and our call today in favor of using evaluate::evaluate() with custom handler.

This could live as its own feature though.

@averissimo

Copy link
Copy Markdown
Contributor Author

Closed in favor of #259

@averissimo averissimo closed this Jun 25, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 25, 2025
@averissimo
averissimo deleted the last_value@teal_reportable branch June 25, 2025 15:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants