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

[Docs] Refine the vignettes #199

Merged
merged 22 commits into from
Feb 16, 2024
Merged

[Docs] Refine the vignettes #199

merged 22 commits into from
Feb 16, 2024

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Feb 15, 2024

Closes #185

Changes:

  1. Updated the diagram so it's more clear on what teal.transform does.
  2. Added a small vignette explaining how to create data_extract_spec
  3. Update the existing vignette's example code without teal.widgets dependency as it's not needed.
  4. Added a small note at the top of every vignette warning that the functions might change.
  5. Refined the vignettes
  6. Add the screenshot of the app outputs in the examples.

vignettes/app-developer.Rmd Outdated Show resolved Hide resolved
vignettes/app-developer.Rmd Outdated Show resolved Hide resolved
vignettes/app-developer.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract.Rmd Outdated Show resolved Hide resolved
vedhav and others added 2 commits February 15, 2024 19:26
Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

I like the changes, but after seeing the PR I get the feeling the window to get this reviewed and merged is too narrow.

As the vignettes would benefit from more details that would delay the release to CRAN.

On the other hand, adding that detail to deprecated it soon™ may not warrant doing it.

vignettes/app-developer.Rmd Outdated Show resolved Hide resolved
vignettes/app-developer.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract.Rmd Outdated Show resolved Hide resolved
vignettes/app-developer.Rmd Outdated Show resolved Hide resolved
@vedhav
Copy link
Contributor Author

vedhav commented Feb 15, 2024

@chlebowa @averissimo What do you both think about removing the app-developer completely?
Because only teal.modules.clinical uses teal.transform and I think right now it only seems to support choices_select which might be removed after the refactor too and we can write the vignette about this during that time.
I will address other comments and update the getting started vignette a little.

@vedhav vedhav requested a review from chlebowa February 15, 2024 17:12
@averissimo
Copy link
Contributor

What do you both think about removing the app-developer completely?

Agree! 👍

@chlebowa
Copy link
Contributor

What do you both think about removing the app-developer completely?

Agreed 👍

@vedhav vedhav marked this pull request as ready for review February 16, 2024 11:40
Copy link
Contributor

github-actions bot commented Feb 16, 2024

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%   19-28, 65, 67, 69, 102-345
R/check_selector.R                   33       0  100.00%
R/choices_labeled.R                 153      27  82.35%   68, 74, 79, 86, 102, 221-225, 229-234, 354-355, 357, 363, 390-397
R/choices_selected.R                 81      11  86.42%   210-238, 275
R/column_functions.R                  3       3  0.00%    15-18
R/data_extract_datanames.R           30       8  73.33%   16-20, 83-85
R/data_extract_filter_module.R      102      47  53.92%   91-104, 106-107, 109-126, 142-161
R/data_extract_module.R             298      67  77.52%   129, 134, 151, 154-159, 161, 180-183, 213-259, 499, 504, 686, 697-698, 776-781
R/data_extract_read_module.R        137       7  94.89%   34, 39-41, 43, 138, 155
R/data_extract_select_module.R       32      18  43.75%   29-46
R/data_extract_single_module.R       60       2  96.67%   30, 43
R/data_extract_spec.R                32       0  100.00%
R/filter_spec.R                     186       1  99.46%   280
R/format_data_extract.R              16       1  93.75%   47
R/get_dplyr_call.R                  297       0  100.00%
R/get_merge_call.R                  278      29  89.57%   32-38, 49, 215-224, 391, 407-419
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   17-18
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  134       6  95.52%   123, 249-253
R/merge_expression_module.R          60      11  81.67%   161-166, 184, 362-367
R/Queue.R                            23       0  100.00%
R/resolve_delayed.R                  16       4  75.00%   77-80
R/resolve.R                         113      44  61.06%   179-285
R/select_spec.R                      64       8  87.50%   99, 179-186
R/utils.R                            37      24  35.14%   33-46, 179-192
R/zzz.R                               2       2  0.00%    2-3
TOTAL                              2362     446  81.12%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 0c121f8

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Feb 16, 2024

Unit Tests Summary

  1 files   24 suites   6s ⏱️
189 tests 189 ✅ 0 💤 0 ❌
659 runs  659 ✅ 0 💤 0 ❌

Results for commit 0c121f8.

♻️ This comment has been updated with latest results.

vignettes/data-extract-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-extract.Rmd Outdated Show resolved Hide resolved
vignettes/data-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-merge.Rmd Outdated Show resolved Hide resolved
vignettes/data-merge.Rmd Outdated Show resolved Hide resolved
vedhav and others added 8 commits February 16, 2024 17:41
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
@vedhav vedhav force-pushed the 185-refine-vignettes@main branch 2 times, most recently from 6bc3343 to 7814441 Compare February 16, 2024 12:59
@vedhav vedhav force-pushed the 185-refine-vignettes@main branch from 7814441 to 809427e Compare February 16, 2024 13:00
@vedhav vedhav requested a review from chlebowa February 16, 2024 13:16
@vedhav vedhav enabled auto-merge (squash) February 16, 2024 13:16
@vedhav vedhav force-pushed the 185-refine-vignettes@main branch from 81fb811 to 94487ab Compare February 16, 2024 14:32
Copy link
Contributor

@chlebowa chlebowa left a comment

Choose a reason for hiding this comment

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

Well done @vedhav 👍

vignettes/data-extract.Rmd Show resolved Hide resolved
@vedhav vedhav merged commit 848921f into main Feb 16, 2024
24 checks passed
@vedhav vedhav deleted the 185-refine-vignettes@main branch February 16, 2024 16:14
@vedhav vedhav mentioned this pull request Feb 16, 2024
vedhav added a commit that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] Refine the vignettes
4 participants