Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

library calls possible within a call #157

Merged
merged 2 commits into from
Oct 25, 2023
Merged

library calls possible within a call #157

merged 2 commits into from
Oct 25, 2023

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Oct 25, 2023

Possible to include library call in eval_code

#93

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2023

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ------------------------
R/include_css_js.R               7       7  0.00%    12-20
R/qenv-concat.R                 10       0  100.00%
R/qenv-constructor.R            12       0  100.00%
R/qenv-eval_code.R              52       2  96.15%   100, 109
R/qenv-get_code.R               18       0  100.00%
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%    16
R/qenv-within.R                  7       0  100.00%
R/utils-code_dependency.R      190       7  96.32%   37, 42, 252-253, 317-320
R/utils.R                       12       0  100.00%
TOTAL                          398      17  95.73%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/qenv-eval_code.R       +3       0  +0.24%
TOTAL                    +3       0  +0.03%

Results for commit: 5ea82b4

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@chlebowa
Copy link
Contributor

chlebowa commented Oct 25, 2023

What if someone detaches a package that is in the middle of the search path (for whatever reason)?

@gogonzo
Copy link
Contributor Author

gogonzo commented Oct 25, 2023

What if someone detaches a package that si in the middle of the search path (for whatever reason)?

as far as one uses eval_code to do so it's okey from reproducibility perspective

@github-actions
Copy link
Contributor

Unit Tests Summary

    1 files    10 suites   1s ⏱️
  92 tests   92 ✔️ 0 💤 0
185 runs  185 ✔️ 0 💤 0

Results for commit 5ea82b4.

@chlebowa
Copy link
Contributor

But you can then lose a function from scope and the condition won't be triggered. I can't decide if that's going to be a problem 🤔

@gogonzo
Copy link
Contributor Author

gogonzo commented Oct 25, 2023

But you can then lose a function from scope and the condition won't be triggered. I can't decide if that's going to be a problem 🤔

Removing namespace and then trying to obtain object from this namespace is a bug introduced by user. In the interactive session this would fail as well. It doesn't seem to be a qenv problem.


Trick I proposed also prevent us from the problem of detached parent. Parent will be reset again

@chlebowa
Copy link
Contributor

Sounds good 👍

@gogonzo gogonzo added the core label Oct 25, 2023
@gogonzo gogonzo merged commit d5c8aae into main Oct 25, 2023
21 checks passed
@gogonzo gogonzo deleted the 93_library_calls@main branch October 25, 2023 15:04
@chlebowa
Copy link
Contributor

We haven't considered that using this feature forces us to call teal.data (in practice probably teal) before packages required for pre-processing.
Instead of

  library(scda)
  library(scda.2022)
  library(dplyr)
  library(teal.modules.clinical)
  library(teal.modules.general)
  library(teal.osprey)
  library(nestcolor)
  # optional libraries
  library(sparkline)

one would do

  library(teal)
  library(scda)
  library(scda.2022)
  library(dplyr)
  library(teal.modules.clinical)
  library(teal.modules.general)
  library(teal.osprey)
  library(nestcolor)
  # optional libraries
  library(sparkline)

This leaves us with a different search path:

                             old                           new
1                     .GlobalEnv                    .GlobalEnv
2              package:sparkline             package:sparkline
3              package:nestcolor             package:nestcolor
4            package:teal.osprey           package:teal.osprey
5                 package:osprey                package:osprey
6   package:teal.modules.general  package:teal.modules.general
7              package:shinyTree             package:shinyTree
8               package:ggmosaic              package:ggmosaic
9                package:ggplot2               package:ggplot2
10 package:teal.modules.clinical package:teal.modules.clinical
11                  package:tern                  package:tern
12               package:rtables               package:rtables
13            package:formatters            package:formatters
14                  package:teal                 package:dplyr
15        package:teal.transform             package:scda.2022
16              package:magrittr                  package:scda
17            package:teal.slice                  package:teal
18             package:teal.data        package:teal.transform
19                 package:shiny              package:magrittr
20                 package:dplyr            package:teal.slice
21             package:scda.2022             package:teal.data
22                  package:scda                 package:shiny
23                 tools:rstudio                 tools:rstudio
24                 package:stats                 package:stats
25              package:graphics              package:graphics
26             package:grDevices             package:grDevices
27                 package:utils                 package:utils
28              package:datasets              package:datasets
29               package:methods               package:methods
30                     Autoloads                     Autoloads
31                  package:base                  package:base

I don't think this should be a problem but it is something we should be aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants