Skip to content

Conversation

@m7pr
Copy link
Contributor

@m7pr m7pr commented Aug 5, 2025

Fixes #262 and insightsengineering/teal.gallery#218

Alternative solution: #263

Functions were kept on the left side of the dependency graph, which lead to cyclic unwanted dependencies.
Now all functions, even when used on the LHS are moved to the RHS of the dependency graph.

devtools::load_all(".")
devtools::load_all("../teal.data")
data <- teal_data()
data <- within(data, {
  library(random.cdisc.data)
  library(nestcolor)
  
  ADSL <- radsl(seed = 1)
  ADMH <- radmh(ADSL, seed = 1)
  ADVS <- radvs(ADSL, seed = 1)
  teal.data::col_labels(ADMH[c("MHDISTAT")]) <- c("Status of Disease")
  
  ADVS <- dplyr::inner_join(x = ADVS, y = ADSL[, c("STUDYID", "USUBJID"), drop = FALSE], by = c("STUDYID", "USUBJID"))
  
})
data@code
cat(get_code(data, names = "ADVS"))
library(random.cdisc.data)
library(nestcolor)
ADSL <- radsl(seed = 1)
ADVS <- radvs(ADSL, seed = 1)
ADVS <- dplyr::inner_join(x = ADVS, y = ADSL[, c("STUDYID", "USUBJID"), drop = FALSE], by = c("STUDYID", "USUBJID"))

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2025

badge

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                  60       1  98.33%   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                    26       0  100.00%
R/qenv-get_warnings.R                5       0  100.00%
R/qenv-join.R                        7       7  0.00%    137-151
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      254       3  98.82%   160, 258, 331
R/utils.R                           42       0  100.00%
TOTAL                              581      46  92.08%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  -------
R/utils-get_code_dependency.R       +7      +1  -0.37%
TOTAL                               +7      +1  -0.08%

Results for commit: 02b1b3b

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2025

Unit Tests Summary

  1 files   14 suites   6s ⏱️
171 tests 168 ✅ 3 💤 0 ❌
261 runs  258 ✅ 3 💤 0 ❌

Results for commit 02b1b3b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test demonstrating recursive loop (reason for the PR)

@m7pr m7pr requested a review from Copilot August 6, 2025 12:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes improper code dependency handling by preventing functions from being positioned on the left-hand side of dependency graphs, which was causing unwanted cyclic dependencies. The solution moves all functions to the right-hand side of the dependency graph, even when they appear on the LHS of assignments.

  • Introduces a new function move_functions_after_arrow to handle function positioning in dependency chains
  • Modifies the extract_occurrence function to apply the function movement logic

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2025

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_get_code 👶 $+0.07$ doesn_t_consider_function_called_on_the_lhs_as_a_dependent_in_this_call_dependency_in_further_calls_

Results for commit f2a1b19

♻️ This comment has been updated with latest results.

@m7pr m7pr requested a review from gogonzo August 6, 2025 12:55
m7pr and others added 6 commits August 6, 2025 16:39
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge after fixing cicd

m7pr and others added 2 commits August 7, 2025 10:57
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
@m7pr m7pr enabled auto-merge (squash) August 7, 2025 09:01
@m7pr m7pr merged commit 3af8c95 into main Aug 7, 2025
26 checks passed
@m7pr m7pr deleted the 262_get_code@main branch August 7, 2025 12:05
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Improper code dependency

3 participants