211 `[.teal_data` S3 method #346

m7pr · 2024-10-28T13:10:33Z

Companion to insightsengineering/teal.code#216

Introduces [.teal_data S3 method for teal_data that is based on [.qenv but also limits join_keys.

github-actions · 2024-10-28T13:13:02Z

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  --------------------
R/cdisc_data.R                   1       0  100.00%
R/deprecated.R                  57      57  0.00%    19-344
R/dummy_function.R               2       2  0.00%    14-15
R/formatters_var_labels.R       61       0  100.00%
R/join_key.R                    38       0  100.00%
R/join_keys-c.R                 12       0  100.00%
R/join_keys-extract.R          128       0  100.00%
R/join_keys-names.R             15       0  100.00%
R/join_keys-parents.R           30       0  100.00%
R/join_keys-print.R             45       0  100.00%
R/join_keys-utils.R             73       3  95.89%   35-38
R/join_keys.R                   22       0  100.00%
R/teal_data-class.R             26       1  96.15%   69
R/teal_data-constructor.R       30      16  46.67%   33, 36-42, 52-58, 61
R/teal_data-datanames.R         20       0  100.00%
R/teal_data-extract.R           17       6  64.71%   37-42
R/teal_data-get_code.R          13       8  38.46%   112-118, 122
R/teal_data-show.R               4       4  0.00%    14-19
R/testhat-helpers.R             26       0  100.00%
R/topological_sort.R            32       0  100.00%
R/verify.R                      42      11  73.81%   65, 95-99, 102-106
TOTAL                          694     108  84.44%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  -------
R/teal_data-constructor.R      +30     +16  +46.67%
R/teal_data-extract.R          +17      +6  +64.71%
TOTAL                          +47     +22  -0.50%

Results for commit: 1c2f05a

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2024-10-28T13:13:30Z

Unit Tests Summary

1 files 13 suites 1s ⏱️
162 tests 162 ✅ 0 💤 0 ❌
219 runs 219 ✅ 0 💤 0 ❌

Results for commit 967c1c5.

github-actions · 2024-10-28T13:13:32Z

Unit Tests Summary

1 files 14 suites 1s ⏱️
167 tests 167 ✅ 0 💤 0 ❌
226 runs 226 ✅ 0 💤 0 ❌

Results for commit 1c2f05a.

♻️ This comment has been updated with latest results.

llrs-roche

Once I read in detail the code and documentation I have some feedback.
I think the examples will be very helpful, I am not sure if the current changes on documentation have the desired consequences.

More importantly matching base's generic argument and take advantage of the S3 methods used here will probably make developing and maintaining the code easier.

When I checked locally the examples failed (It could be my setup), but running interactively shows it too (so there might be some undeclared expectations/dependencies):

d> data["a"]
Error en get(x, envir = ns, inherits = FALSE): 
  objeto '[.qenv' no encontrado
d> traceback()
4: get(x, envir = ns, inherits = FALSE)
3: utils::getFromNamespace("[.qenv", "teal.code") at teal_data-extract.R#33
2: `[.teal_data`(data, "a")
1: data["a"]
d> packageVersion("teal.code")
[1] '0.5.0.9011'
d> packageVersion("teal.data")
[1] '0.6.0.9015'

R/teal_data-extract.R

llrs-roche · 2024-10-28T15:49:30Z

R/teal_data-extract.R

+#' `x[names]` subsets objects in `teal_data` environment and limit the code to the necessary needed to build limited
+#' objects.
+#'
+#' @param names (`character`) names of objects included in `teal_subset` to subset


Perhaps this is inherited from [.qenv but usually subset uses i argument to reuse the generic base extractor.

yes, but with i it works for numeric input, and we only support character input :)

Then in my opinion, the natural way to handle this would be to convert the numeric input to names and use that internally, easier for the user and simple for the developer. But it can also provide an informative error message to users. Users will try to use numbers given that this is what usually R accepts. Sorry, to raise a new thing, could you add a test for numeric subsetting and NA_character_? I know we get the Assertion error, but I want to make sure this is tested/documented for users.

@llrs-roche oh yeah, but we don't want to introduce anything for numeric as qenv is basically an environment that is not subsettable for numeric as it does not have a natural order (like atomic vectors have).

library(teal.code) q <- qenv() q <- eval_code(q, "a <- 1; b <- 2") q@env[1] # > Error in q@env[1] : object of type 'environment' is not subsettable q@env[[1]] # > Error in q@env[[1]] : wrong arguments for subsetting an environment q@env[["a"]]

Numbers are fine for atomic vectors, or simple S3 objects that naturally use indexes, but I don't think it is the case for the environmemnt. We actually don't want it specifically as we only allow extraction by name, not by numeric index, so that you specifically need to provide the name of the object. With numeric input you can get a different element, depending on the current state of the object : )

Sorry, to raise a new thing, could you add a test for numeric subsetting and NA_character_? I know we get the Assertion error, but I want to make sure this is tested/documented for users.

No worries, good idea : )

Here are extended tests 5f17af0

R/teal_data-extract.R

m7pr · 2024-10-29T08:10:00Z

@llrs-roche

objeto '[.qenv' no encontrado

Any chance you can change your local setup to English : P ?

Sys.setlocale("LC_ALL","English")

https://stackoverflow.com/questions/16347731/how-to-change-the-locale-of-r

m7pr · 2024-10-29T08:13:19Z

@llrs-roche you need to install teal.code from 211_subset@main, the same as teal.data from 211_subset@main. Looks like you did not had teal.code installed from 211_subset@main, hence you could not find a function [.qenv.
This is how we prepare PRs for multiple repositories - if functionality is shared between multiple packages, then you need to install all packages from a specific branch to run and check the code.

Thanks for the attempt!

R/teal_data-extract.R

github-actions · 2024-10-29T10:38:33Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
extract	👶		$+0.07$	$+7$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	$±Time$	Test Case
extract	👶	$+0.01$	_._handles_empty_names
extract	👶	$+0.01$	_.handles_names_as_NA_character
extract	👶	$+0.04$	_._returns_limited_join_keys
extract	👶	$+0.01$	_._thorws_warnings_if_names_is_numeric
extract	👶	$+0.01$	_._throws_warning_if_names_is_NULL

Results for commit c5d725d

♻️ This comment has been updated with latest results.

llrs-roche

Thanks for the changes. I think this is mostly ready for merging, I just added a petition to have a test for a numeric and NA_character_.

I think, the workaround with NextMethod() come from defining it differently than the generic, but seem easy enough.

I tried loading teal.code@211_subset@main and running [.qenv exampels still got the same problems object 'parsed_code' not found, so it might be something related to my environment.

R/teal_data-extract.R

llrs-roche · 2024-10-29T16:52:37Z

R/teal_data-extract.R

+#' `x[names]` subsets objects in `teal_data` environment and limit the code to the necessary needed to build limited
+#' objects.
+#'
+#' @param names (`character`) names of objects included in `teal_subset` to subset


Then in my opinion, the natural way to handle this would be to convert the numeric input to names and use that internally, easier for the user and simple for the developer. But it can also provide an informative error message to users. Users will try to use numbers given that this is what usually R accepts. Sorry, to raise a new thing, could you add a test for numeric subsetting and NA_character_? I know we get the Assertion error, but I want to make sure this is tested/documented for users.

R/teal_data-extract.R

…ing/teal.data into 211_subset@main

m7pr · 2024-10-30T10:21:31Z

I tried loading teal.code@211_subset@main and running [.qenv exampels still got the same problems object 'parsed_code' not found, so it might be something related to my environment.

And you did good. I actually left teal.code in unstable state yesterday when I left the office insightsengineering/teal.code#216 (comment) so it was broken. You got the very same error as I got : )

gogonzo

Minor comments

R/teal_data-extract.R

tests/testthat/test-extract.R

R/teal_data-constructor.R

Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: Marcin <[email protected]>

llrs-roche · 2024-10-30T13:31:32Z

Feel free to merge but I still have problems with executing the code with the latest update from teal.code@211_subset@main:

d> data <- teal_data()
d> data <- eval_code(data, "a <- 1;b<-2")
Error in parse(text = code_split[[i]], keep.source = FALSE) : 
  <text>:1:12: unexpected symbol
1: a <- 1;b<-2a
               ^
d> data["a"]
✅︎ verified teal_data object
<environment: 0x00000171ebb781d0> [L]
Parent: <environment: devtools_shims>
Warning message:
In `[.teal_data`(data, "a") :
  None of `names` elements exist in `teal_data`. Returning empty `teal_data`.
d> data[c("a", "b")]
✅︎ verified teal_data object
<environment: 0x00000171ea8c9710> [L]
Parent: <environment: devtools_shims>
Warning message:
In `[.teal_data`(data, c("a", "b")) :
  None of `names` elements exist in `teal_data`. Returning empty `teal_data`.
d> 
d> session_info()
─ Session info ───────────────────────────────────────────────────────────────────────
 setting  value
 version  R version 4.4.1 (2024-06-14 ucrt)
 os       Windows 11 x64 (build 22631)
 system   x86_64, mingw32
 ui       RStudio
 language en
 collate  Spanish_Spain.utf8
 ctype    Spanish_Spain.utf8
 tz       Europe/Madrid
 date     2024-10-30
 rstudio  2024.09.0+375 Cranberry Hibiscus (desktop)
 pandoc   3.5 @ C:\\Users\\sanchosl\\AppData\\Local\\Pandoc\\pandoc.exe

─ Packages ───────────────────────────────────────────────────────────────────────────
 !  package     * version    date (UTC) lib source
    backports     1.5.0      2024-05-23 [2] CRAN (R 4.4.0)
    brio          1.1.5      2024-04-24 [2] CRAN (R 4.4.1)
    cachem        1.1.0      2024-05-16 [2] CRAN (R 4.4.1)
    callr         3.7.6      2024-03-25 [2] CRAN (R 4.4.1)
    checkmate     2.3.2      2024-07-29 [2] CRAN (R 4.4.1)
    cli           3.6.3      2024-06-21 [2] CRAN (R 4.4.1)
    curl          5.2.3      2024-09-20 [2] CRAN (R 4.4.1)
    desc          1.4.3      2023-12-10 [2] CRAN (R 4.4.1)
    devtools    * 2.4.5      2022-10-11 [2] CRAN (R 4.4.1)
    digest        0.6.37     2024-08-19 [2] CRAN (R 4.4.1)
    ellipsis      0.3.2      2021-04-29 [2] CRAN (R 4.4.1)
    fastmap       1.2.0      2024-05-15 [2] CRAN (R 4.4.1)
    fs            1.6.4      2024-04-25 [2] CRAN (R 4.4.1)
    glue          1.8.0      2024-09-30 [2] CRAN (R 4.4.1)
    htmltools     0.5.8.1    2024-04-04 [2] CRAN (R 4.4.1)
    htmlwidgets   1.6.4      2023-12-06 [2] CRAN (R 4.4.1)
    httpuv        1.6.15     2024-03-26 [2] CRAN (R 4.4.1)
    later         1.3.2      2023-12-06 [2] CRAN (R 4.4.1)
    lifecycle     1.0.4      2023-11-07 [2] CRAN (R 4.4.1)
    magrittr      2.0.3      2022-03-30 [2] CRAN (R 4.4.1)
    memoise       2.0.1      2021-11-26 [2] CRAN (R 4.4.1)
    mime          0.12       2021-09-28 [2] CRAN (R 4.4.0)
    miniUI        0.1.1.1    2018-05-18 [2] CRAN (R 4.4.1)
    pkgbuild      1.4.4      2024-03-17 [2] CRAN (R 4.4.1)
    pkgload       1.4.0      2024-06-28 [2] CRAN (R 4.4.1)
    processx      3.8.4      2024-03-16 [2] CRAN (R 4.4.1)
    profvis       0.4.0      2024-09-20 [2] CRAN (R 4.4.1)
    promises      1.3.0      2024-04-05 [2] CRAN (R 4.4.1)
    ps            1.8.0      2024-09-12 [2] CRAN (R 4.4.1)
    purrr         1.0.2      2023-08-10 [2] CRAN (R 4.4.1)
    R6            2.5.1      2021-08-19 [2] CRAN (R 4.4.1)
    Rcpp          1.0.13     2024-07-17 [2] CRAN (R 4.4.1)
    remotes       2.5.0      2024-03-17 [2] CRAN (R 4.4.1)
    rlang         1.1.4      2024-06-04 [2] CRAN (R 4.4.1)
    rprojroot     2.0.4      2023-11-05 [2] CRAN (R 4.4.1)
    rstudioapi    0.17.1     2024-10-22 [2] CRAN (R 4.4.1)
    sessioninfo   1.2.2      2021-12-06 [2] CRAN (R 4.4.1)
    shiny         1.9.1      2024-08-01 [2] CRAN (R 4.4.1)
    teal.code   * 0.5.0.9012 2024-10-30 [1] Github (insightsengineering/teal.code@a32939c)
 VP teal.data   * 0.6.0.9015 2024-10-21 [?] Github (insightsengineering/teal.data@b5ccb1d) (on disk 0.6.0.9012)
    testthat    * 3.2.1.1    2024-04-14 [2] CRAN (R 4.4.1)
    urlchecker    1.0.1      2021-11-30 [2] CRAN (R 4.4.1)
    usethis     * 3.0.0      2024-07-29 [2] CRAN (R 4.4.1)
    vctrs         0.6.5      2023-12-01 [2] CRAN (R 4.4.1)
    withr         3.0.1      2024-07-31 [2] CRAN (R 4.4.1)
    xtable        1.8-4      2019-04-21 [2] CRAN (R 4.4.1)

 [1] C:/Users/sanchosl/R-dev
 [2] C:/Users/sanchosl/AppData/Local/R/win-library/4.4
 [3] C:/Program Files/R/R-4.4.1/library

 V ── Loaded and on-disk version mismatch.
 P ── Loaded and on-disk path mismatch.

R/teal_data-constructor.R

Signed-off-by: Marcin <[email protected]>

R/teal_data-extract.R

…ing/teal.data into 211_subset@main

[.teal_data S3 method

967c1c5

m7pr added the core label Oct 28, 2024

m7pr mentioned this pull request Oct 28, 2024

211 [.qenv S3 method + replacement of @id, @warnings, and @messages fields insightsengineering/teal.code#216

Draft

10 tasks

provide prefix for utils::

362b982

donyunardi assigned llrs-roche Oct 28, 2024

llrs-roche reviewed Oct 28, 2024

View reviewed changes

m7pr added 3 commits October 29, 2024 09:36

fixing rdname

a2610a3

rename teal_data.R to teal_data-constructor.R

c01612f

an attempt with NextMethod

fd4fe27

m7pr commented Oct 29, 2024

View reviewed changes

R/teal_data-extract.R Outdated Show resolved Hide resolved

m7pr and others added 4 commits October 29, 2024 10:21

add a warning

f7894e5

extend vignettes with [ for teal_data

e53afc8

write tests for [.teal_data

1ea9d8f

[skip style] [skip vbump] Restyle files

220e46c

llrs-roche approved these changes Oct 29, 2024

View reviewed changes

m7pr added 4 commits October 30, 2024 11:10

extned tests for different input class

5f17af0

Merge branch '211_subset@main' of https://github.com/insightsengineer…

6d3c3e8

…ing/teal.data into 211_subset@main

remove NextMethod comments

39d9f37

extend tests with expect_warning

b6c6955

gogonzo reviewed Oct 30, 2024

View reviewed changes

m7pr and others added 2 commits October 30, 2024 14:27

Update R/teal_data-extract.R

009bbc0

Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: Marcin <[email protected]>

Update tests/testthat/test-extract.R

fc0d0c0

Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: Marcin <[email protected]>

remove unneeded roxygen2 tags

f74635a

m7pr commented Oct 30, 2024

View reviewed changes

R/teal_data-constructor.R Outdated Show resolved Hide resolved

m7pr and others added 4 commits October 30, 2024 14:34

Update R/teal_data-constructor.R

d3eb395

Signed-off-by: Marcin <[email protected]>

warn about skipped names

5839f0d

move tests to teal.code

269b77b

[skip style] [skip vbump] Restyle files

1c2f05a

pawelru reviewed Oct 31, 2024

View reviewed changes

R/teal_data-extract.R Outdated Show resolved Hide resolved

m7pr added 2 commits October 31, 2024 15:12

simplify teal_code extract as it reuses qenv extract

5b1e1a8

Merge branch '211_subset@main' of https://github.com/insightsengineer…

7fd89a1

…ing/teal.data into 211_subset@main

m7pr requested a review from llrs-roche October 31, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

211 `[.teal_data` S3 method #346

211 `[.teal_data` S3 method #346

m7pr commented Oct 28, 2024

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024

github-actions bot commented Oct 28, 2024 •

edited

Loading

llrs-roche left a comment

llrs-roche Oct 28, 2024

m7pr Oct 29, 2024

llrs-roche Oct 29, 2024

m7pr Oct 30, 2024

m7pr Oct 30, 2024

m7pr Oct 30, 2024

m7pr commented Oct 29, 2024

m7pr commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

llrs-roche left a comment

llrs-roche Oct 29, 2024

m7pr commented Oct 30, 2024

gogonzo left a comment

llrs-roche commented Oct 30, 2024

211 [.teal_data S3 method #346

Are you sure you want to change the base?

211 [.teal_data S3 method #346

Conversation

m7pr commented Oct 28, 2024

github-actions bot commented Oct 28, 2024 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Oct 28, 2024

Unit Tests Summary

github-actions bot commented Oct 28, 2024 • edited Loading

Unit Tests Summary

llrs-roche left a comment

Choose a reason for hiding this comment

llrs-roche Oct 28, 2024

Choose a reason for hiding this comment

m7pr Oct 29, 2024

Choose a reason for hiding this comment

llrs-roche Oct 29, 2024

Choose a reason for hiding this comment

m7pr Oct 30, 2024

Choose a reason for hiding this comment

m7pr Oct 30, 2024

Choose a reason for hiding this comment

m7pr Oct 30, 2024

Choose a reason for hiding this comment

m7pr commented Oct 29, 2024

m7pr commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 • edited Loading

Unit Test Performance Difference

llrs-roche left a comment

Choose a reason for hiding this comment

llrs-roche Oct 29, 2024

Choose a reason for hiding this comment

m7pr commented Oct 30, 2024

gogonzo left a comment

Choose a reason for hiding this comment

llrs-roche commented Oct 30, 2024

211 `[.teal_data` S3 method #346

211 `[.teal_data` S3 method #346

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading