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.

Sign up for GitHub

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

Jump to bottom

added required columns to docs #149

Merged
merged 8 commits into from
Aug 9, 2023
Merged

added required columns to docs #149

merged 8 commits into from
Aug 9, 2023

Conversation

denisovan31415
Copy link
Contributor

part of #121

added the required columns in the roxgyen docs.

And also similarly mentioned the required columns in the gosahawk app of teal.gallery:

https://code.roche.com/nest/r-packages/teal.gallery/-/merge_requests/18

@denisovan31415 denisovan31415 added documentation Improvements or additions to documentation core labels May 24, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 24, 2022 •
edited
Loading

Code Coverage Summary

Filename                                 Stmts    Miss  Cover    Missing
-------------------------------------  -------  ------  -------  ---------
R/tm_g_gh_boxplot.R                        301     301  0.00%    216-573
R/tm_g_gh_correlationplot.R                509     509  0.00%    284-893
R/tm_g_gh_density_distribution_plot.R      233     233  0.00%    163-437
R/tm_g_gh_lineplot.R                       496     496  0.00%    179-758
R/tm_g_gh_scatterplot.R                    191     191  0.00%    163-398
R/tm_g_gh_spaghettiplot.R                  214     214  0.00%    228-490
R/toggleable_slider.R                      141     141  0.00%    82-239
R/utils.R                                  306     306  0.00%    2-489
R/zzz.R                                      1       1  0.00%    2
TOTAL                                     2392    2392  0.00%

Results for commit: 9b0bd95

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@denisovan31415 denisovan31415 requested a review from npaszty May 24, 2022 12:05
nikolas-burkoff
nikolas-burkoff reviewed May 24, 2022
View reviewed changes
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff left a comment

Choose a reason for hiding this comment

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

From a coding perspective this is fine - should probably wait for Nick P to approve before merging

npaszty
npaszty reviewed May 24, 2022
View reviewed changes
R/tm_g_gh_lineplot.R Outdated
@@ -5,7 +5,7 @@
#' @inheritParams teal.widgets::standard_layout
#' @param label menu item label of the module in the teal app.
#' @param dataname analysis data passed to the data argument of teal init. E.g. ADaM structured
#' laboratory data frame ADLB.
#' laboratory data frame ADLB. This table must contain the columns `AVISITCD`, `BASE`, `BASE2`, `AVALU`, `LBSTRESC`
Copy link
Contributor

Choose a reason for hiding this comment

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

there are others like LOQFL but this is documented as comments in the app.R code. whats best approach so as not to repeat in header across all modules and functions?
i seem to remember roxygen having ability to read in a template chunk of header info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have included them in the teal_goshawk.R file

Copy link
Contributor

@npaszty npaszty May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@junlueZH
i believe the full list for goshawk/teal.goshawk is

goshawk expects the following variables: AVISITCD, AVALU, BASE2, CHG2, LBSTRESC, LOQFL, PCHG2

should also be in goshawk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@npaszty added

@donyunardi
Copy link
Contributor

Hi @npaszty
Just checking on any old PR with core label.
It looks like Junlue make the changes in teal.gallery already so I assume this PR should be good to go too.
Could you please confirm that we can move forward with the merge?

@npaszty
Copy link
Contributor

npaszty commented Sep 30, 2022

@donyunardi

I added a number of additional comments to the sample app.R file in teal.gallery. Those should be brought into the goshawk and teal.goshawk documentation so that it's easy/intuitive for users to find the information. Currently the addition to teal_goshawk.R is only revealing required variable names but I think it would be more helpful to enhance with what the variables are and the rationale for having added them.

I was thinking that a vignette should be written to document more effectively. Thoughts?

@gogonzo
Copy link
Contributor

gogonzo commented Aug 1, 2023

This PR has 1 year. If it needs a discussion please move it to the issue and close this PR.

@npaszty
Copy link
Contributor

npaszty commented Aug 1, 2023

wow this is old.
still think that the documentation could be enhanced per suggestion above but otherwise sure...
approved.

@gogonzo gogonzo enabled auto-merge (squash) August 9, 2023 11:55
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

badge

Code Coverage Summary

Filename                                 Stmts    Miss  Cover    Missing
-------------------------------------  -------  ------  -------  ---------
R/tm_g_gh_boxplot.R                        349     349  0.00%    216-632
R/tm_g_gh_correlationplot.R                552     552  0.00%    282-940
R/tm_g_gh_density_distribution_plot.R      274     274  0.00%    161-484
R/tm_g_gh_lineplot.R                       559     559  0.00%    184-837
R/tm_g_gh_scatterplot.R                    235     235  0.00%    167-453
R/tm_g_gh_spaghettiplot.R                  288     288  0.00%    230-584
R/toggleable_slider.R                      154     154  0.00%    82-253
R/utils-arbitrary_lines.r                  121     121  0.00%    19-172
R/utils-data_constraints.r                 186     186  0.00%    2-255
R/utils-keep_range_slider_updated.r         23      23  0.00%    8-38
R/utils-maptrt.r                             9       9  0.00%    25-37
R/utils-templ_ui.r                          48      48  0.00%    2-73
R/utils.R                                   17      17  0.00%    12-32
R/zzz.R                                      1       1  0.00%    2
TOTAL                                     2816    2816  0.00%

Diff against main

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

Results for commit: fd3f760

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@gogonzo gogonzo merged commit 205bcf5 into main Aug 9, 2023
@gogonzo gogonzo deleted the 121_lineplot_args@main branch August 9, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikolas-burkoff nikolas-burkoff nikolas-burkoff left review comments

@npaszty npaszty npaszty approved these changes

Assignees
No one assigned
Labels
core documentation Improvements or additions to documentation priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@denisovan31415 @donyunardi @npaszty @gogonzo @nikolas-burkoff @kpagacz