Skip to content

165 trim trace for qenv.error@main - #183

Merged
m7pr merged 9 commits into
mainfrom
165_trim_trace_for_qenv.error@main
Dec 15, 2023
Merged

165 trim trace for qenv.error@main#183
m7pr merged 9 commits into
mainfrom
165_trim_trace_for_qenv.error@main

Conversation

@m7pr

@m7pr m7pr commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

Close #165

@m7pr
m7pr requested a review from chlebowa December 11, 2023 11:28
@m7pr m7pr added the core label Dec 11, 2023
@m7pr
m7pr marked this pull request as ready for review December 11, 2023 12:57
@github-actions

github-actions Bot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

Unit Tests Summary

    1 files      9 suites   0s ⏱️
  62 tests   62 ✔️ 0 💤 0
134 runs  134 ✔️ 0 💤 0

Results for commit 71116d7.

♻️ This comment has been updated with latest results.

Comment thread R/qenv-get_code.R
@github-actions

github-actions Bot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ---------
R/qenv-concat.R             10       0  100.00%
R/qenv-constructor.R        16      13  18.75%   54-93
R/qenv-eval_code.R          52       2  96.15%   100, 109
R/qenv-get_code.R           20       1  95.00%   38
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%    19
R/qenv-within.R              8       0  100.00%
R/utils.R                   10       0  100.00%
TOTAL                      206      17  91.75%

Diff against main

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

Results for commit: 71116d7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@m7pr
m7pr requested a review from pawelru December 12, 2023 08:02
@m7pr
m7pr enabled auto-merge (squash) December 12, 2023 08:02
@chlebowa
chlebowa disabled auto-merge December 15, 2023 10:30

@chlebowa chlebowa 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.

> devtools::load_all()
ℹ Loading teal.code
> qe <- qenv() |> within(i <- iris) |> within(stop("oh no"))
> qe
<qenv.error: oh no 
 when evaluating qenv code:
stop("oh no")>
> get_code(qe)
Error: oh no 
 when evaluating qenv code:
stop("oh no")

trace: 
 i <- iris
 stop("oh no")

This is what I intended 👍

As long as you're here, would you mind modifying this line in get_code like it was changed in teal_data::get_code?

# from
parse(text = paste(c("{", object@code, "}"), collapse = "\n"), keep.source = TRUE)
# to
parse(text = object@code, keep.source = TRUE)

I disabled auto-merge to give you chance.

@m7pr

m7pr commented Dec 15, 2023

Copy link
Copy Markdown
Contributor Author

Hey @chlebowa why would we want to change this?

image

The line

parse(text = paste(c("{", object@code, "}"), collapse = "\n"), keep.source = TRUE)

was introduced in here #176 so that resulted expression (for deparse = FALSE) has a length 1, so it's consistent with the length of the result for deparse = TRUE #176 (comment)

@chlebowa

Copy link
Copy Markdown
Contributor

before:

> devtools::load_all()
ℹ Loading teal.code
> qe <- qenv() |> within(i <- iris) |> within(m <- mtcars)
> get_code(qe, deparse = TRUE)
[1] "i <- iris\nm <- mtcars"
>

after:

> devtools::load_all()
ℹ Loading teal.code
> qe <- qenv() |> within(i <- iris) |> within(m <- mtcars)
> get_code(qe, deparse = TRUE)
[1] "i <- iris\nm <- mtcars"
> 

@m7pr

m7pr commented Dec 15, 2023

Copy link
Copy Markdown
Contributor Author

Hey @chlebowa your before and after are the same. The line you mentioned is for deparse = FALSE

# parse(text = object@code, keep.source = TRUE) case
qe <- qenv() |> within(i <- iris) |> within(m <- mtcars)
get_code(qe, deparse = FALSE)
# > expression(i <- iris, m <- mtcars)
length(get_code(qe, deparse = FALSE))
# > [1] 2
# parse(text = paste(c("{", object@code, "}"), collapse = "\n"), keep.source = TRUE) case
qe <- qenv() |> within(i <- iris) |> within(m <- mtcars)
get_code(qe, deparse = FALSE)
# > expression({
# > i <- iris
# > m <- mtcars
# > })
length(get_code(qe, deparse = FALSE))
# > [1] 1

@m7pr

m7pr commented Dec 15, 2023

Copy link
Copy Markdown
Contributor Author

If we are trying to be consistent with what's in https://github.com/insightsengineering/teal.data/blob/main/R/teal_data-get_code.R#L60 then I would actually modify what's in teal.data::get_code.teal_data so it returns output of length 1 for each deparse = FALSE

@m7pr

m7pr commented Dec 15, 2023

Copy link
Copy Markdown
Contributor Author

get_code in teal_data is inconsistent in terms of the length of the output if you want to have a deparsed or non-deparsed output

library(teal.data)
tdata1 <- teal_data()
tdata1 <- within(tdata1, {
  a <- 1
  b <- a^5
  c <- list(x = 2)
})

length(get_code(tdata1, deparse = FALSE))
# > [1] 3
length(get_code(tdata1, deparse = TRUE))
# > [1] 1

@chlebowa

Copy link
Copy Markdown
Contributor

I stand corrected, I ran my tests wrong.

Merge away.

@m7pr

m7pr commented Dec 15, 2023

Copy link
Copy Markdown
Contributor Author

No worries, I made an issue in teal.code to follow-up on this one insightsengineering/teal.data#224

@m7pr
m7pr merged commit 18e4e41 into main Dec 15, 2023
@m7pr
m7pr deleted the 165_trim_trace_for_qenv.error@main branch December 15, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

code storage in qenv.error

3 participants