175 ls(qenv)

[m7pr]

Close #175

[github-actions]

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_env.R             3       3  0.00%    23-31
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                      209      20  90.43%

Diff against main

Filename            Stmts    Miss  Cover
----------------  -------  ------  --------
R/qenv-get_env.R       +3      +3  +100.00%
TOTAL                  +3      +3  -1.32%

Results for commit: d725efc

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

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

Results for commit d725efc.

♻️ This comment has been updated with latest results.

[pawelru]

This is good from my perspective and I like the approach for exposing slot for further use.
Please just raise awareness of this within the team as there is a conflicting PR.

[pawelru]

I like this that much that I would even go with this to the README and vignettes. Please just make sure to align what's printed to avoid e.g. the following

qenv_2
#> <environment: 0x00000135b544cfe8> [L]
#> Parent: <environment: package:teal.code>
#> Bindings:
#> • x: <dbl> [L]
#> • y: <dbl> [L]
#> • z: <dbl> [L]

get_env(qenv_2)
<some other hash printed>

[m7pr]

This is good from my perspective and I like the approach for exposing slot for further use.
Please just raise awareness of this within the team as there is a conflicting PR.

Just marked this solution in here insightsengineering/teal#1004 (comment)

[m7pr]

@pawelru

Please just make sure to align what's printed to avoid e.g. the following

What exactly we should avoid? Printing qenv at all or what do you mean?

[chlebowa]

I think @pawelru wanted to make sure the same env hash is printed but that is a given because get_env just takes the env from the slot and returns it as is.

You could add a unit test for env identity but I think that's going a bit too far, there are no operations, it would be testing @.

[m7pr]

Yeah, this crossed my mind but just wanted to double check

[m7pr]

I think this might be an overkill. So qenv has a show method (that works as a print for S4 objects) that is implemented as

setMethod("show", "qenv", function(object) {
  rlang::env_print(object@env)
})

So for qenv we basically have a nice way of printing what's in qenv@env. And now we have a get_env for which we would like to get another nice print? So get_env should return an object of some class for which we would use rlang::env_print( for it's print or show methods. I think that is too much.

[chlebowa]

I believe get_env should return an environment (which it does) and there is a built-in method to print that. I wouldn't change anything else here.

[m7pr]

Yeah, agree. @pawelru would you like us to extend README and vignettes with just an usage of get_env(object) after each print/show(object) where an output would be a vector of names of objects included in object@env?

[pawelru]

Yeah, agree. @pawelru would you like us to extend README and vignettes with just an usage of get_env(object) after each print/show(object) where an output would be a vector of names of objects included in object@env?

Yes please. A very simple example would be great

[m7pr]

Amazing, I did extend README and vignettes. Examples in README did not work as qenv() does not allow to specify any arguments anymore so it's great we had a chance to test :)