Fix bug, add tests and update NEWS

[llrs-roche]

Pull Request

Fixes #293

This is a minimal patch (mostly generated with AI) that fixes the test cases by skipping objects that are on the LHS.

This has revealed several issues that I'm considering what to do with it:

• extract_dependency() list objects not present (cyl column on the first test added).
• duplicated parsing of code on extract_dependency()

[github-actions]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------
R/qenv-c.R                          55       0  100.00%
R/qenv-class.R                      13       0  100.00%
R/qenv-concat.R                      7       0  100.00%
R/qenv-constructor.R                 1       0  100.00%
R/qenv-errors.R                      4       4  0.00%    6-9
R/qenv-eval_code.R                  63       1  98.41%   40
R/qenv-extract.R                    30       0  100.00%
R/qenv-get_code.R                   24       0  100.00%
R/qenv-get_env.R                     3       1  66.67%   27
R/qenv-get_messages.r                5       0  100.00%
R/qenv-get_outputs.R                 6       0  100.00%
R/qenv-get_var.R                    13       1  92.31%   13
R/qenv-get_warnings.R                5       0  100.00%
R/qenv-join.R                        1       1  0.00%    13
R/qenv-length.R                      2       1  50.00%   2
R/qenv-show.R                       29      29  0.00%    19-50
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      258       3  98.84%   160, 258, 326
R/utils.R                           42       0  100.00%
TOTAL                              569      41  92.79%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  -------
R/utils-get_code_dependency.R       -8       0  -0.03%
TOTAL                               -8       0  -0.10%

Results for commit: 5db9b00

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   14 suites   7s ⏱️
189 tests 185 ✅ 4 💤 0 ❌
273 runs  269 ✅ 4 💤 0 ❌

Results for commit 5db9b00.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
utils-get_code_dependency	👶		$+0.06$	get_code_with_subsetting_same_object
utils-get_code_dependency	👶		$+0.05$	get_code_with_subsetting_same_object_with_composed_logic

Results for commit c6b7730

♻️ This comment has been updated with latest results.

[averissimo]

This block is not very good, it works, but barely. It doesn't make sense to replace first instance after assignment, it would make sense to replace first instance in parenthesis.

See example below that incorrectly replaces first occurrence (the one not in parenthesis)

browser() statement was placed just before returned modified result

pkgload::load_all(".")

data <- qenv()
data <- within(data, {
  mtcars <- datasets::mtcars
  #mtcars <- mtcars[mtcars$cyl == 4, ]
  aa <- seq_len(nrow(mtcars))
  mtcars <- mtcars$mpg > 15 & aa[mtcars$mpg & mtcars$cyl]
  # mtcars <- mtcars[mtcars$cyl == 4, ]
})
get_code(data, names = "mtcars") |> cat()

Could we simplify the code to just return c(ans, in_parenthesis(pd))

We don't really care about having repeated element, or do we?

diff --git a/R/utils-get_code_dependency.R b/R/utils-get_code_dependency.R
index 1bd38ef0..5b2ac04d 100644
--- a/R/utils-get_code_dependency.R
+++ b/R/utils-get_code_dependency.R
@@ -304,24 +304,7 @@ extract_occurrence <- function(pd) {
   after <- match(min(x$id[assign_cond]), sort(x$id[c(min(assign_cond), sym_cond)])) - 1
   ans <- append(x[sym_cond, "text"], "<-", after = max(1, after))
   ans <- move_functions_after_arrow(ans, unique(x[sym_fc_cond, "text"]))
-  roll <- in_parenthesis(pd)
-  if (length(roll)) {
-    # detect elements appeared in parenthesis and move them on RHS
-    # but only their first appearance after assignment
-    # as the same object can appear as regular object and the one used in parenthesis
-    result <- ans
-    arrow_idx <- which(result == "<-")
-    for (elem in roll) {
-      idx <- which(result == elem)
-      idx <- idx[idx > arrow_idx][1]
-      if (!is.na(idx)) {
-        result <- result[-idx]
-      }
-    }
-    c(result, roll)
-  } else {
-    ans
-  }
+  c(ans, in_parenthesis(pd))
 }
 
 #' Moves function names to the right side of dependency graph

[llrs-roche]

Great simplification! I think we do care about the position of the elements and using unique() would alter that, I only used it for the in_parenthesis. I like your approach much more than mine, it removes code without a side effect.

[averissimo]

💯