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

Decouple scda #139

Merged
merged 11 commits into from
Jul 6, 2023
Merged

Decouple scda #139

merged 11 commits into from
Jul 6, 2023

Conversation

kartikeyakirar
Copy link
Contributor

fixes #133

@kartikeyakirar
Copy link
Contributor Author

linking this issue; #140

@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

Unit Tests Summary

    1 files    25 suites   10s ⏱️
207 tests 206 ✔️ 1 💤 0
708 runs  707 ✔️ 1 💤 0

Results for commit f808c40.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------
R/all_choices.R                       1       0  100.00%
R/call_utils.R                      156     124  20.51%   14-23, 64, 66, 68, 107-431
R/check_selector.R                   31       0  100.00%
R/choices_labeled.R                 200      54  73.00%   59, 65, 70, 77, 93, 215-219, 223-228, 258-271, 383-384, 386, 434-466
R/choices_selected.R                 81      11  86.42%   200-228, 259
R/column_functions.R                  3       3  0.00%    13-16
R/data_extract_datanames.R           32       8  75.00%   9-13, 64-66
R/data_extract_filter_module.R       95      16  83.16%   75-88, 90-91, 145
R/data_extract_module.R             298      66  77.85%   3, 115, 120, 137, 140-145, 147, 166-169, 197-243, 477, 482, 661, 727-732
R/data_extract_read_module.R        135       7  94.81%   29, 33-36, 131, 148
R/data_extract_select_module.R       32      18  43.75%   31-48
R/data_extract_single_module.R       53       2  96.23%   29, 42
R/data_extract_spec.R                32       0  100.00%
R/data_merge_module.R                61      15  75.41%   101-118
R/filter_spec.R                     186       1  99.46%   302
R/format_data_extract.R              16       1  93.75%   49
R/get_dplyr_call.R                  299       0  100.00%
R/get_merge_call.R                  280      29  89.64%   28-34, 45, 206-215, 369, 385-397
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   18-19
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  135       6  95.56%   77, 225-229
R/merge_expression_module.R          49       1  97.96%   147
R/Queue.R                            23       0  100.00%
R/resolve_delayed.R                  16       0  100.00%
R/resolve.R                         114      44  61.40%   227-310
R/select_spec.R                      49       8  83.67%   103, 167-174
R/utils.R                            24      11  54.17%   26-39
R/zzz.R                               2       2  0.00%    2-3
TOTAL                              2421     429  82.28%

Diff against main

Filename               Stmts    Miss  Cover
-------------------  -------  ------  -------
R/choices_labeled.R        0      -7  +3.50%
TOTAL                      0      -7  +0.29%

Results for commit: 86cf39d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@kartikeyakirar kartikeyakirar marked this pull request as draft July 3, 2023 18:56
@gogonzo
Copy link
Contributor

gogonzo commented Jul 4, 2023

@kartikeyakirar you can remove all the tests which depends on scda_dataset_connector (if that's an obstacle)

@kartikeyakirar kartikeyakirar marked this pull request as ready for review July 4, 2023 06:59
@gogonzo gogonzo self-assigned this Jul 4, 2023
Comment on lines 1 to 2
adlb <- rADLB # nolint
adtte <- rADTTE # nolint
Copy link
Contributor

@chlebowa chlebowa Jul 4, 2023

Choose a reason for hiding this comment

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

Suggested change
adlb <- rADLB # nolint
adtte <- rADTTE # nolint
ADLB <- rADLB # nolint
ADTTE <- rADTTE # nolint

Let us keep the capitalization of data set names.
Also, that is what the # nolint is for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Regarding uppercase dataset names in teal. In teal.data when using cdisc_data there is a check if "ADSL" exists, which is necessary for a join keys to work. It means we have this limitation in whole teal framework and we should stick to uppercase always when dealing with ADaM datasets. Other (then ADaM) datasets can be written in lowercase.

I think in the future we should make teal.data not being case-sensitive.

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 wait with merge. We need to merge all packages from the same branch in the same time. We will be ready to merge them once this is approved by @lcd2yyz insightsengineering/osprey#121

@gogonzo gogonzo merged commit 2cdf5e4 into main Jul 6, 2023
26 checks passed
@gogonzo gogonzo deleted the decouple_scda@main branch July 6, 2023 08:04
gogonzo added a commit to insightsengineering/scda that referenced this pull request Jul 6, 2023
- [ ] teal insightsengineering/teal#858
- [ ] teal.widgets
- [ ] osprey insightsengineering/osprey#121
- [ ] teal.osprey
insightsengineering/teal.osprey#214
- [ ] teal.transform
insightsengineering/teal.transform#139

---------

Signed-off-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>
Co-authored-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
gogonzo added a commit to insightsengineering/scda.2022 that referenced this pull request Jul 6, 2023
@m7pr m7pr mentioned this pull request Jul 12, 2023
m7pr added a commit that referenced this pull request Jul 12, 2023
A follow-up after #139 that closed #133 

After I had a chance to do a scda decoupling for goshawk
insightsengineering/goshawk#198 and had a chance
to review scda decoupling for teal.modules.general
insightsengineering/teal.modules.general#534
I realized some changes need to be applied in PRs that were already
merged in other packages.

Main changes:
- I prepend dataset names with package names as now we are having the
same data in multiple packages (`teal.transform::rADAE` and
`teal.modules.general::rADAE` for example)
- added a `data-raw/data.R` file to show how the `data/` folder was
created
- extended `.RBuildignore` file to omit `data-raw/data.R` while building
the package
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.

Decouple scda from teal.transform
3 participants