Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

111 lookup valid check #119

Merged
merged 2 commits into from
Mar 19, 2022
Merged

111 lookup valid check #119

merged 2 commits into from
Mar 19, 2022

Conversation

yli110-stat697
Copy link
Contributor

close #111

@github-actions
Copy link
Contributor

Code Coverage Summary

Filename         Stmts    Miss  Cover    Missing
-------------  -------  ------  -------  --------------------------------------------------------------------------------------
R/radae.R          172       3  98.26%   82, 101, 143
R/radaette.R       211       4  98.10%   52, 73, 79, 243
R/radcm.R          137      27  80.29%   53, 87, 158-184
R/raddv.R           91       3  96.70%   41, 56, 140
R/radeg.R          269       1  99.63%   185
R/radex.R          187       1  99.47%   225
R/radhy.R          136       1  99.26%   197
R/radlb.R          253       1  99.60%   221
R/radmh.R           90       2  97.78%   48, 87
R/radpc.R           57       1  98.25%   97
R/radpp.R           50       1  98.00%   99
R/radqs.R          107       1  99.07%   132
R/radrs.R          121       3  97.52%   49, 56, 156
R/radsaftte.R        1       1  0.00%    17
R/radsl.R          156       2  98.72%   48, 225
R/radsub.R         142       1  99.30%   159
R/radtr.R          127       0  100.00%
R/radtte.R         124       4  96.77%   53, 73, 85, 128
R/radvs.R          160       1  99.38%   174
R/utils.R          154      59  61.69%   9-12, 56-59, 111-112, 117-118, 155-156, 214, 264, 266, 295-301, 305, 338-408, 466, 470
TOTAL             2745     117  95.74%

Results for commit: 74622f7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

Unit Tests Summary

  1 files    1 suites   18s ⏱️
21 tests 21 ✔️ 0 💤 0 ❌
54 runs  54 ✔️ 0 💤 0 ❌

Results for commit 813b059.

yli110-stat697
yli110-stat697 commented Mar 18, 2022
View reviewed changes
R/radeg.R
@@ -66,6 +66,7 @@ radeg <- function(ADSL, # nolint
checkmate::assert_integer(n_assessments, len = 1, any.missing = FALSE)
checkmate::assert_integer(n_days, len = 1, any.missing = FALSE)
checkmate::assert_integer(max_n_eg, len = 1, any.missing = FALSE)
checkmate::assert_data_frame(lookup, null.ok = TRUE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the function has the lookup parameter, but didn't use it. Should we remove it? @shajoezhu

yli110-stat697
yli110-stat697 commented Mar 18, 2022
View reviewed changes
R/radex.R
@@ -64,6 +64,7 @@ radex <- function(ADSL, # nolint
checkmate::assert_integer(n_assessments, len = 1, any.missing = FALSE)
checkmate::assert_integer(n_days, len = 1, any.missing = FALSE)
checkmate::assert_integer(max_n_exs, len = 1, any.missing = FALSE)
checkmate::assert_data_frame(lookup, null.ok = TRUE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, the parameter lookup wasn't in use in this function

yli110-stat697
yli110-stat697 commented Mar 18, 2022
View reviewed changes
R/radlb.R
@@ -68,6 +68,7 @@ radlb <- function(ADSL, # nolint
checkmate::assert_integer(n_assessments, len = 1, any.missing = FALSE)
checkmate::assert_integer(n_days, len = 1, any.missing = FALSE)
checkmate::assert_integer(max_n_lbs, len = 1, any.missing = FALSE)
checkmate::assert_data_frame(lookup, null.ok = TRUE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment above

@shajoezhu shajoezhu enabled auto-merge (squash) March 19, 2022 06:50
shajoezhu
shajoezhu approved these changes Mar 19, 2022
View reviewed changes
Copy link
Contributor

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

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

Thanks a lot @yli110-stat697 ! lgtm! Let's investigate how to add look up for radex, radeg, and radlb in #121

@shajoezhu shajoezhu merged commit 3888c8f into main Mar 19, 2022
@shajoezhu shajoezhu deleted the 111_lookup_valid_check@main branch March 19, 2022 06:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@shajoezhu shajoezhu shajoezhu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve argument validity checking lookup
2 participants
@yli110-stat697 @shajoezhu