Adds `names(qenv/qenv.error)` S3 methods #218

averissimo · 2024-10-28T13:38:00Z

Pull Request

Changes description

qenv S4 class inherits from environment data class
Removes @env slot in favor of qenv
Replace all instances of @env with @.xData (slot created by parent class)

github-actions · 2024-10-28T17:12:11Z

Unit Tests Summary

1 files 10 suites 1s ⏱️
123 tests 121 ✅ 2 💤 0 ❌
220 runs 218 ✅ 2 💤 0 ❌

Results for commit faa843b.

♻️ This comment has been updated with latest results.

github-actions · 2024-10-28T17:12:14Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
qenv-class	👶		$+0.23$	$+5$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on `main`	$±Time$	Test Case
qenv-class	👶		$+0.05$	creates_a_locked_environment
qenv-class	👶		$+0.06$	creates_a_locked_environment_when_.xData_is_manually_defined
qenv-class	👶		$+0.01$	throws_error_when_.xData_is_not_an_environment
qenv-class	👶		$+0.10$	throws_error_when_id_and_code_length_doesn_t_match
qenv_constructor	👶		$+0.01$	does_not_allow_binding_to_be_added
qenv_constructor	👶		$+0.01$	does_not_allow_binding_to_be_modified
qenv_constructor	👶		$+0.02$	is_an_environment
qenv_constructor	👶		$+0.00$	ls_does_not_show_hidden_objects
qenv_constructor	👶		$+0.00$	ls_shows_available_objets
qenv_constructor	👶		$+0.00$	ls_shows_nothing_on_empty_environment
qenv_constructor	👶		$+0.00$	names_show_all_objects
qenv_constructor	💀	$0.01$	$-0.01$	parent_of_qenv_environment_is_locked
qenv_constructor	💀	$0.00$	$-0.00$	parent_of_qenv_environment_is_the_parent_of_.GlobalEnv
qenv_constructor	👶		$+0.01$	qenv_environment_is_locked
qenv_constructor	👶		$+0.00$	via_qenv_directly
qenv_constructor	👶		$+0.00$	via_slot
qenv_eval_code	👶		$+0.01$	.xData_in_qenv_is_always_a_sibling_of.GlobalEnv
qenv_eval_code	💀	$0.01$	$-0.01$	env_in_qenv_is_always_a_sibling_of.GlobalEnv
qenv_get_code	💀	$0.01$	$-0.01$	works_for_datanames_of_length_1
qenv_get_code	👶		$+0.01$	works_for_names_of_length_1
qenv_get_var	👶		$+0.01$	get_var_and_only_returns_objects_from_qenv_not_.GlobalEnv
qenv_within	👶		$+0.00$	multiline_expressions_are_evaluated
qenv_within	👶		$+0.01$	within.qenv_renturns_a_qenv_where_.xData_is_a_deep_copy_of_that_in_data_
qenv_within	💀	$0.01$	$-0.01$	within.qenv_renturns_a_qenv_where_env_is_a_deep_copy_of_that_in_data_

Results for commit dd2a739

♻️ This comment has been updated with latest results.

github-actions · 2024-10-28T17:12:38Z

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ---------
R/qenv-class.R                       5       0  100.00%
R/qenv-concat.R                     10       0  100.00%
R/qenv-constructor.R                18      17  5.56%    76-119
R/qenv-errors.R                      4       4  0.00%    6-9
R/qenv-eval_code.R                  52       2  96.15%   99, 108
R/qenv-get_code.R                   28       0  100.00%
R/qenv-get_env.R                     3       1  66.67%   29
R/qenv-get_var.R                    27       0  100.00%
R/qenv-get_warnings.R               24       0  100.00%
R/qenv-join.R                       64       2  96.88%   211, 223
R/qenv-length.R                      2       0  100.00%
R/qenv-show.R                        1       1  0.00%    19
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      191       1  99.48%   283
R/utils.R                           10       0  100.00%
TOTAL                              447      28  93.74%

Diff against main

Filename                Stmts    Miss  Cover
--------------------  -------  ------  --------
R/qenv-class.R             +5       0  +100.00%
R/qenv-constructor.R       +2      +4  -13.19%
R/qenv-errors.R            +4      +4  +100.00%
R/qenv-get_env.R            0      -2  +66.67%
R/qenv-get_var.R           +8       0  +100.00%
R/qenv-join.R             +18      +2  -3.12%
R/qenv-length.R            +2       0  +100.00%
TOTAL                     +39      +8  -1.36%

Results for commit: faa843b

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

explanatory

gogonzo · 2024-10-29T07:21:51Z

R/qenv-names.R

+  ls(get_env(x))
+}
+
+#' @rdname names.qenv


We need to decide where to put names.qenv. At this moment within, eval_code, get_code are part of qenv documentation and they are described in the sections. Problem with some methods (names, within) is that they have generic in base and it is not obvious that one needs to call names.qenv.

gogonzo

Please test cover new functions

…vior to [[

gogonzo · 2024-10-29T17:11:34Z

NAMESPACE

 S3method("[[",qenv.error)
+S3method("[[<-",qenv.error)


Why is it needed?

I'd like to preface that I'm more than happy to remove as this is **totally** nitpicking.

My reasoning adding this would be to make the API consistent

The 2 snippets below shows the **released/main** behavior and the assignments (<-) seem inconsistent with qenv.

It seems odd that with qenv.error the script runs a few more commands.

qenv object without errors

library(teal.code) q <- qenv() |> within(a <- 2) q$something <- 2 #> Error in `$<-`(`*tmp*`, something, value = 2): no method for assigning subsets of this S4 class q[["another"]] <- 3 #> Error in `[[<-`(`*tmp*`, "another", value = 3): [[<- defined for objects of type "S4" only for subclasses of environment q$something #> Error in q$something: $ operator not defined for this S4 class q[["another"]] #> object 'another' not found #> NULL

^{Created on 2024-10-29 with reprex v2.1.1}

qenv object when there's an error in within code

library(teal.code) q <- qenv() |> within(stop()) q$something <- 2 q[["another"]] <- 3 q$something #> [1] 2 q[["another"]] #> Error: #> when evaluating qenv code: #> stop()

^{Created on 2024-10-29 with reprex v2.1.1}

We don't need to fence all possible methods. There is large set of the methods inherited from environment, it is important that officially supported functions are mentioned in the docs of qenv. We need to mention inherited working methods but not add them. What I would add though to qenv.error is $.qenv.error, [[.qenv.error, as.list.qenv.error, ls.qenv.error

ls.qenv.error cannot be added, but it will still give an error, so no harm there

hmm, there is a difference between throwing error and returning error. ls(<qenv.error>) will throw while names.qenv.error(<qenv.error>) will return error. The first one needs to be tryCatched while second one not. We need to check if ls(data()) in teal internals doesn't throw because of this (probably not as teal is protected against any types of errors)

averissimo

Some topics to discuss on this implementation

averissimo · 2024-10-29T17:36:33Z

R/qenv-eval_code.R

@@ -31,7 +31,7 @@ setMethod("eval_code", signature = c("qenv", "character"), function(object, code
  id <- sample.int(.Machine$integer.max, size = 1)

  object@id <- c(object@id, id)
-  object@env <- rlang::env_clone(object@env, parent = parent.env(.GlobalEnv))
+  object@.xData <- rlang::env_clone(object@.xData, parent = parent.env(.GlobalEnv))


{rlang} functions don't play nicely with this approach, so we need to access the slot directly

🔴 Could this lack of support from {rlang} be a blocker for this new approach?

pkgload::load_all("teal.code") #> ℹ Loading teal.code q1 <- qenv() rlang::is_environment(q1) #> [1] FALSE base::is.environment(q1) #> [1] TRUE rlang::env_clone(q1) #> Error in `rlang::env_clone()`: #> ! `env` must be an environment, not a <qenv> object. rlang::is_environment #> function(x) { #> typeof(x) == "environment" #> } #> <bytecode: 0x65127a0d6b08> #> <environment: namespace:rlang> typeof(q1) #> [1] "S4"

^{Created on 2024-10-29 with reprex v2.1.1}

averissimo · 2024-10-29T17:39:19Z

R/qenv-get_var.R

@@ -55,3 +55,29 @@ setMethod("[[", signature = c("qenv", "ANY"), function(x, i) {
    class = c("validation", "try-error", "simpleError")
  ))
 }
+
+#' @export
+`$.qenv.error` <- function(x, name) {


🔴 Should we add this method and have it behave just like [[?

We need to allow $ to retrieve existing properties of qenv.error, as conditionMessage(x) will try to access x$message, x$call and x$trace.

averissimo · 2024-10-29T17:39:53Z

R/qenv-get_var.R

+    return(NextMethod("$", x))
+  }
+
+  class(x) <- setdiff(class(x), "qenv.error")


ℹ️ Safeguard to prevent infinite recursion, this can be "safely" removed if reviewer deems necessary

averissimo · 2024-10-29T17:42:33Z

tests/testthat/test-qenv_constructor.R

@@ -1,19 +1,67 @@
+testthat::describe("qenv inherits from environment: ", {


Please test cover new functions

ℹ️ Added tests that addressed last comment before the "change" (covering a wider range of functionalities)

gogonzo

There are some @env remaining in R/ directory . Also there shouldn't be any mention about internal slots (regex:#'.+@(env|.xData|code|id|warnings|messages) in the documentation @ - please replace them with something implicit.
(you can leave join docs unchanged - this is a bigger problem)

gogonzo · 2024-10-31T06:47:53Z

R/qenv-constructor.R

@@ -23,9 +45,7 @@
 #'


I'd add usage of names, [[ and $ to the examples section somewhere before # retrieve code. This could be a "section" # extracting objects from qenv

See 0a644988135a098665-af7054b837b.

I may have gone a bit overboard by including these methods in the usage section, WDYT?

tests/testthat/test-qenv_within.R

R/qenv-constructor.R

Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: André Veríssimo <[email protected]>

R/qenv-get_var.R

feat: initial support for names(teal_data)

bfa8390

averissimo added the core label Oct 28, 2024

averissimo mentioned this pull request Oct 28, 2024

Adds names() function and deprecates datanames() insightsengineering/teal.data#347

Draft

8 tasks

docs: update NEWS

127bc7c

averissimo marked this pull request as ready for review October 28, 2024 17:09

averissimo added 6 commits October 28, 2024 17:13

docs: minor change

cb2cc57

fix: warning in R CMD check

72f0595

docs: typo

9a6343a

fix: remove implementation of names()<- as error message is self

e1a69b3

explanatory

fix: remove extra arguments for names, not supported

c335f52

fix: remove extra word from wordlist

873a1b6

gogonzo self-assigned this Oct 29, 2024

gogonzo reviewed Oct 29, 2024

View reviewed changes

averissimo and others added 6 commits October 29, 2024 15:42

feat: qenv inherits from environment class

a203f81

Merge branch 'main' into 333_deprecate_datanames@main

fd93704

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

8b855f3

fix: improves tests and adds $ getter to qenv.error with similar beha…

4efca24

…vior to [[

feat: prevent assignment to qenv.error

0a02726

doc: update news

281c994

gogonzo reviewed Oct 29, 2024

View reviewed changes

averissimo added 2 commits October 29, 2024 17:30

fix: problem when printing qenv.error

b1ffbe7

docs: adds .xData slot documentation

e457a4c

averissimo commented Oct 29, 2024

View reviewed changes

averissimo added 3 commits October 29, 2024 17:44

chore: cleanup of previous implementation of names.qenv

2c76a8a

chore: lintr cleanup

ab9e342

feat: expand on compatibility with an environment

c56a5ca

averissimo added 9 commits October 30, 2024 15:38

fix: complete tests for qenv-class

9904570

doc: adds section to qenv constructor

5c0ef7d

fix: test and adds extra protection on qenv validation

558bd72

fix: move constructor logic to "initialize" method of qenv

4ec3e9f

fix: problem with integer (1L) shorthand in within

e00fd92

test: problem with integer (1L) shorthand in within

be480f1

fix: order and formal of callNextMethod

b7d1885

fix: minor bugs

2a32022

chore: fix lintr

9049379

gogonzo reviewed Oct 31, 2024

View reviewed changes

averissimo and others added 8 commits October 31, 2024 17:56

Apply suggestions from code review

bb5c5fb

Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: André Veríssimo <[email protected]>

docs: update

709265f

docs: small improvements

1fe8b18

[skip style] [skip vbump] Restyle files

3ae0541

Update tests/testthat/test-qenv_within.R

0a64498

Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: André Veríssimo <[email protected]>

docs: implements @gogonzo suggestions and cleans up docs

d4ee6d0

docs: move section around

af7054b

docs: superseded

c668d98

averissimo commented Oct 31, 2024

View reviewed changes

R/qenv-get_var.R Show resolved Hide resolved

fix: use newlines in code parseing on multiline expression with within

faa843b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `names(qenv/qenv.error)` S3 methods #218

Adds `names(qenv/qenv.error)` S3 methods #218

averissimo commented Oct 28, 2024 •

edited by gogonzo

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

gogonzo Oct 29, 2024

gogonzo left a comment

gogonzo Oct 29, 2024

averissimo Oct 29, 2024

gogonzo Oct 30, 2024

averissimo Oct 30, 2024

gogonzo Oct 31, 2024

averissimo left a comment

averissimo Oct 29, 2024 •

edited

Loading

averissimo Oct 29, 2024 •

edited

Loading

averissimo Oct 29, 2024 •

edited

Loading

averissimo Oct 29, 2024 •

edited

Loading

gogonzo left a comment •

edited

Loading

gogonzo Oct 31, 2024

averissimo Oct 31, 2024

		@@ -1,19 +1,67 @@
		testthat::describe("qenv inherits from environment: ", {

		S3method("[[",qenv.error)
		S3method("[[<-",qenv.error)

Adds names(qenv/qenv.error) S3 methods #218

Are you sure you want to change the base?

Adds names(qenv/qenv.error) S3 methods #218

Conversation

averissimo commented Oct 28, 2024 • edited by gogonzo Loading

Pull Request

Changes description

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

Unit Tests Summary

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

Unit Test Performance Difference

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

Code Coverage Summary

Diff against main

Choose a reason for hiding this comment

gogonzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qenv object without errors

qenv object when there's an error in within code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

averissimo left a comment

Choose a reason for hiding this comment

averissimo Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

averissimo Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

averissimo Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

averissimo Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

gogonzo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adds `names(qenv/qenv.error)` S3 methods #218

Adds `names(qenv/qenv.error)` S3 methods #218

averissimo commented Oct 28, 2024 •

edited by gogonzo

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

`qenv` object without errors

`qenv` object when there's an error in `within` code

averissimo Oct 29, 2024 •

edited

Loading

averissimo Oct 29, 2024 •

edited

Loading

averissimo Oct 29, 2024 •

edited

Loading

averissimo Oct 29, 2024 •

edited

Loading

gogonzo left a comment •

edited

Loading