Skip to content

127 introduce within.qenv - #149

Merged
chlebowa merged 24 commits into
mainfrom
127_within@main
Oct 5, 2023
Merged

127 introduce within.qenv#149
chlebowa merged 24 commits into
mainfrom
127_within@main

Conversation

@chlebowa

@chlebowa chlebowa commented Oct 4, 2023

Copy link
Copy Markdown
Contributor

Closes #127

Added an S3 within method for class qenv which is a wrapper for eval_code.

Usage: within(data, expr, ...)

within.qenv is created for convenience and accepts only inline expressions, not language or character variables, nor literal strings.

The ... argument can be used to inject values into expr before evaluation.

EXAMPLES

Evaluate simple or compound expressions in qenv.

q <- new_qenv()
q <- within(q, i <- iris)
q <- within(q, {
  m <- mtcars
  f <- faithful
})
q
within(q, dim(f))

Inject value into expr.

qq <- within(q, ii <- subset(i, Species == species), species = "virginica")
within(qq, print(summary(ii)))

Inject value with variable.

input_value <- "versicolor"
qq <- within(q, ii <- subset(i, Species == species), species = input_value)
within(qq, print(summary(ii)))

@chlebowa chlebowa added the core label Oct 4, 2023
@chlebowa
chlebowa requested a review from gogonzo October 4, 2023 11:12
@chlebowa
chlebowa marked this pull request as ready for review October 4, 2023 11:12
@github-actions

github-actions Bot commented Oct 4, 2023

Copy link
Copy Markdown
Contributor

badge

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%   91, 100
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/qenv-within.R              7       0  100.00%
R/utils.R                   19       0  100.00%
TOTAL                      211      10  95.26%

Diff against main

Filename           Stmts    Miss  Cover
---------------  -------  ------  --------
R/qenv-within.R       +7       0  +100.00%
TOTAL                 +7       0  +0.16%

Results for commit: eea0513

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@chlebowa
chlebowa requested a review from m7pr October 4, 2023 11:15
@github-actions

github-actions Bot commented Oct 4, 2023

Copy link
Copy Markdown
Contributor

Unit Tests Summary

    1 files      9 suites   1s ⏱️
  72 tests   72 ✔️ 0 💤 0
157 runs  157 ✔️ 0 💤 0

Results for commit eea0513.

♻️ This comment has been updated with latest results.

@chlebowa

chlebowa commented Oct 5, 2023

Copy link
Copy Markdown
Contributor Author

@gogonzo In the current form within cannot accept language objects or lists of language objects. If one really wants to squeeze an expression there, one can use do.call.

# single expression
expr <- expression(i <- iris, m <- mtcars)
do.call(within, list(q, expr))

# list of expressions
exprlist <- list(expression(i <- iris), expression(m <- mtcars))
do.call(within, list(q, do.call(c, exprlist)))

(Calls and call lists will not work, it has to be a single expression.)

The problem(?) is substitution is not available in these cases. The question is: do we care? Because I can make it work but then within itself will be open for directly passing expressions (possibly calls as well). I guess a use case would be something like what happens in tmc, where a module builds up a list of expressions and pushes them into a qenv all at once, but then the expressions are already properly substituted. On the other hand, the substitutions have to be done for each expression separately, whereas with this we could have them all done in one place, which would be neat, wouldn't it?

@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 that warning is not needed for single symbol

Comment thread R/qenv-within.R Outdated
Comment thread R/qenv-within.R
Comment thread R/qenv-within.R Outdated
Comment thread tests/testthat/test-qenv-within.R Outdated
Comment thread tests/testthat/test-qenv-within.R Outdated
Comment thread tests/testthat/test-qenv-within.R Outdated
Comment thread tests/testthat/test-qenv-within.R

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

Please try to write tests with single expectation and precise description.

@gogonzo gogonzo self-assigned this Oct 5, 2023
@chlebowa
chlebowa requested a review from gogonzo October 5, 2023 09:12
Comment thread tests/testthat/test-qenv-within.R Outdated

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

We got it! 🔥 🔥 🔥

@chlebowa
chlebowa merged commit eb9197b into main Oct 5, 2023
@chlebowa
chlebowa deleted the 127_within@main branch October 5, 2023 12:43
@m7pr

m7pr commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

respect

@donyunardi donyunardi mentioned this pull request Aug 15, 2024
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.

Introduce within.qenv

3 participants