Refactoring with checkmate and rlang - #548
Conversation
|
This looks like it will make all of the code more consistent. Thank you!. Please merge in the current origin/main branch and resolve the conflicts. Also, for many of the checkmate calls, you've added |
…ng-refactor # Conflicts: # R/001-add.interval.col.R # R/PKNCA.options.R # R/assertions.R # R/auc.R # R/aucint.R # R/class-PKNCAdata.R # R/class-PKNCAdose.R # R/class-general.R # R/class-summary_PKNCAresults.R # R/exclude.R # R/interpolate.conc.R # R/pk.calc.all.R # R/pk.calc.c0.R # R/superposition.R # R/tss.R # R/tss.monoexponential.R # R/tss.stepwise.linear.R # tests/testthat/test-PKNCA.options.R
| end, | ||
| interval[2] | ||
| ), | ||
| class = "pknca_error_interval_mismatch" |
There was a problem hiding this comment.
Please make sure that all class values differ. This is the same as the one a few lines above, and part of the goal of setting the classes is to be able to differentiate errors by class for programmatic capturing by other tools (e.g. ANCA or ruminate).
| # stricter. | ||
| stop(unit_col, call. = FALSE) | ||
| rlang::abort( | ||
| message = conditionMessage(attr(unit_col, "condition")), |
There was a problem hiding this comment.
conditionMessage() here seems like it is not required. Please remove it or let me know why it is necessary.
| if ("auc.type" %in% names(list(...))) | ||
| stop("auc.type cannot be changed when calling pk.calc.auc.inf, please use pk.calc.auc") | ||
| rlang::abort( | ||
| message = paste( |
There was a problem hiding this comment.
Please do not use paste() as that can make finding the error message harder to diagnose problems.
| if ("auc.type" %in% names(list(...))) | ||
| stop("auc.type cannot be changed when calling pk.calc.auc.all, please use pk.calc.auc") | ||
| rlang::abort( | ||
| message = paste( |
There was a problem hiding this comment.
Same comment about not using paste. Please apply that throughout.
| "auc.type cannot be changed when calling pk.calc.aumc.all,", | ||
| "please use pk.calc.aumc" | ||
| ), | ||
| class = "pknca_error_aumc_type_override" |
There was a problem hiding this comment.
Please make the class unique here, too.
| #' quantification; this is used for AUCinf calculations. If provided as | ||
| #' `clast.obs` (observed clast value, default), AUCinf is AUCinf,obs. If | ||
| #' provided as `clast.pred`, AUCinf is AUCinf,pred. | ||
| #' provided as `clast.pred`, AUCinf is AUCinf,pred.#' |
There was a problem hiding this comment.
An extraneous #' was added to the end of this line. Please remove it.
|
|
||
| test_that("add.interval.col", { | ||
| # Invalid inputs fail | ||
| expect_error( |
There was a problem hiding this comment.
Please reinstate all of the tests where they are using the new regexp, if applicable. (You can remove the "info" part.)
|
Here are the comprehensive findings: Additional review findingsThese supplement the 6 inline comments already on the diff (non-unique classes, Severity token missing from some preexisting condition classes — breaks class-based captureThree internal "report a bug" errors are raised with a
Their siblings correctly use Negative-path tests deleted instead of migrated — coverage regressionPlease restore the tests to match the previous intent. Across ~10 test files, the
The new behavior is now less tested than the old. Please convert these to checkmate semantics quietly differ from the manual checks they replacedPlease add tests for where these tests are stricter: CI can't catch these because the negative tests (finding 2) were deleted. All verified against base:
None look dangerous, but they're undocumented behavior changes with no test pinning them. Please find remaining stop and warning calls"Replaced all
|
…ng-refactor Conflicts: NEWS.md R/001-add.interval.col.R R/aucint.R R/pk.calc.all.R R/pk.calc.urine.R tests/testthat/test-001-add.interval.col.R
Conflicts: NEWS.md
NEWS.md: - Restore the deleted changelog entry for the unsorted concentration-time fix, which a merge dropped while leaving its code and test in place. - Split two bullets that a merge glued together, and put the "Bug Fixes" heading back on its own line so the section renders. - Remove the pk_nca_result_to_df() entry: that regression was introduced and fixed within this branch, so no released version exhibited it. - Document the pknca_* condition class renames under Breaking changes, including the two sparse classes merged into one. Revert the new S3method(AIC,list) registration. AIC.list() keeps @nord, so AIC() dispatch on lists is unchanged from main. Restore the base formatting of aucint.R, pk.calc.c0.R, time.above.R, and AIC.list.R, keeping only the condition-signaling changes. These four were reindented wholesale, which inflated the diff without changing behavior: aucint.R alone was 991 changed lines carrying 22 real ones. The branch diff drops from 4760 changed lines to 3184; the test suite is unchanged at 2745 passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Expanding one-line stop() calls into multi-line rlang::abort() calls left `# nocov` on only some lines of each call, so the unmarked middle lines still counted toward coverage. Per the contributing guide, regions of three or more lines are bracketed with `# nocov start` / `# nocov end`; statements of one or two lines keep a trailing marker on each line. 35 sites across 14 files. Every converted region contains exactly one rlang::abort() or rlang::warn() call and nothing else. Also bracket the closure in zero_len_summary(), where a pre-existing pair of bare `#nocov` markers on the first and last line left the nine-line body counted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…isc_arg The class names were built with sprintf() from the argument name, so the six resulting classes could not be grepped for and only the three pptestcd_cdisc ones were ever evaluated -- a typo in the pptest_cdisc trio would have gone unnoticed. Three literal classes cover both arguments and are exercised by the existing tests. Replace the hand-rolled scalar and name checks with checkmate::test_string() and checkmate::test_names(), keeping rlang::abort() so each failure retains its class. Drop the redundant length(x) != 1 test, which identical(names(x), "route") already implies. Document the validation itself in NEWS: it is stricter than the previous character-or-list check and can reject input that add.interval.col() used to accept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # Validate formalsmap | ||
| checkmate::assert_list( | ||
| x = formalsmap, | ||
| names = if (length(formalsmap) > 0) "unique" else NULL |
There was a problem hiding this comment.
The complex assignment of names here can just be names = "unique" because it is unique if the length is zero.
Simplify to checkmate::assert_lits(x = formalsmap, names = "unique")
| checkmate::assert_number(x = interval[1], na.ok = FALSE, finite = TRUE) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Do not add extra spaces (fix this where changed throughout, but do not do a general pass of all of PKNCA).
| checkmate::assert_character(unit, len = 1) | ||
| checkmate::assert_data_frame(data) | ||
|
|
||
| if (!(unit %in% names(data))) { |
There was a problem hiding this comment.
Use checkmate::assert_names(names(data), must.include = unit) here instead
| ) | ||
| } | ||
|
|
||
| if (!is.character(data[[unit]])) { |
There was a problem hiding this comment.
use checkmate::assert_character(data[[unit]])
| # nocov start | ||
| rlang::abort( | ||
| message = "Unknown error with NA tlast but non-BLQ concentrations", | ||
| class = "pknca_error_internal_tlast" | ||
| ) | ||
| # nocov end |
There was a problem hiding this comment.
Condense this onto one line so that it can have a simpler # nocov. Consider this throughout because a single line with (especially with adding nocov start and end) vs multiple lines can often be more readable. If this causes linting issues, resolve those, too.
Also, all the rlang::abort and rlang::warn calls do not need the message argument named since it is so distinctly the first argument to the call. Do this throughout, too.
| pptestcd_cdisc="AUCALL", | ||
| pptest_cdisc="AUC All" | ||
| ) | ||
| pptest_cdisc="AUC All") |
There was a problem hiding this comment.
Revert moving the parenthesis to the same line. That is an unrelated change. Revert all similar indentation or punctuation movements.
| stopifnot(length(conc) == length(time)) | ||
| checkmate::assert_numeric(conc, any.missing = FALSE) | ||
| checkmate::assert_numeric(time, any.missing = FALSE) | ||
| checkmate::assert_numeric(conc, len = length(time)) |
There was a problem hiding this comment.
The same test from this line can go one line above by asserting that time has len = length(conc) saving a line and being more readable. Consider for similar throughout.
| } else { | ||
| stopifnot(is.numeric(tlast)) | ||
| stopifnot(length(tlast) == 1) | ||
| checkmate::assert_number(tlast) |
There was a problem hiding this comment.
Add the length check back in (len = 1) on this line.
| "tlast was found multiple times" | ||
| } | ||
| rlang::abort( | ||
| message = sprintf( |
There was a problem hiding this comment.
Move the complex message generation outside of the call to rlang::abort. Consider that throughout for anything more complex that e.g. a sprintf or a paste.
There was a problem hiding this comment.
(This is as far as I got in this review pass. Consider all above comments in light of the remaining code and apply consistently.)
Condition signaling:
- Drop the named `message =` argument from every rlang::abort/warn/inform
call (301 sites); it is distinctly the first argument.
- Condense 108 short calls onto one line so `# nocov` can be a trailing
marker rather than a start/end bracket.
- Give every condition class a distinct value (17 sites across 10 reused
names), so callers such as ANCA or ruminate can capture a specific
failure rather than a shared one.
- Replace paste()/paste0() message assembly with sprintf templates (20
sites) so the message a user reports can be grepped for in the source,
and hoist the branching message in choose_interval_method() out of the
abort call.
checkmate:
- assert_unit_col() uses assert_names(must.include=) and
assert_character() in place of two hand-rolled classed aborts.
- add.interval.col() uses assert_list(names = "unique"); the empty-list
conditional was dead because checkmate accepts a zero-length list.
- choose_interval_method() folds the conc/time length check into the
time assertion via len = length(conc).
- pk_nca_result_to_df() reads warning_prep$message directly rather than
through conditionMessage().
Formatting, limited to lines this PR introduced:
- Restore `) {`, drop trailing whitespace, and put closing braces on
their own line (80 lines). Lines unchanged from the base are untouched.
- Remove the leftover `info` arguments from test-001-add.interval.col.R.
Tests follow the renamed classes: the Tobit half-life cases now pin
pknca_warning_halflife_too_few_points_tobit, and the assert_unit_col
cases pin checkmate's messages.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # Re-raise the unit_col error. That is better than unit_value since it is | ||
| # stricter. | ||
| stop(unit_col, call. = FALSE) | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line
| } else { | ||
| stop("Unknown integration method, please report a bug: ", method) # nocov | ||
| # nocov start | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line because it has nocov
| } else if (interval_method_extrap != "zero") { | ||
| stop("Invalid interval_method_extrap, please report a bug: ", interval_method_extrap) # nocov | ||
| # nocov start | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line because it has nocov
| } else if (is.na(clast)) { | ||
| stop("Please report a bug. clast is NA and the half-life is not NA") # nocov | ||
| # nocov start | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line because it has nocov
| paste(missing_times, collapse=", ")) | ||
| } | ||
| warning(warning_message) | ||
| rlang::warn( |
There was a problem hiding this comment.
Simplify this to a single line because it should fit
| } else { | ||
| stop("Invalid 'values' for column specification ", n, " (please report this as a bug).") # nocov | ||
| # nocov start | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line because it has nocov
| # intervals without using add.interval.col) | ||
| stop("Invalid interval definition with no function and multiple dependencies.") # nocov | ||
| # nocov start | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line because it has nocov
| # React to using the default value, if requested | ||
| if (!missing(stop_if_default)) { | ||
| stop(stop_if_default) | ||
| rlang::abort( |
There was a problem hiding this comment.
Simplify this to a single line because it should fit
| ) | ||
| } else if (!missing(warn_if_default)) { | ||
| warning(warn_if_default) | ||
| rlang::warn( |
There was a problem hiding this comment.
Simplify this to a single line
| ) | ||
| } else if (!missing(message_if_default)) { | ||
| message(message_if_default) | ||
| rlang::inform( |
There was a problem hiding this comment.
Simplify this to a single line
Extends the earlier pass, which only recognized a call whose message was a string literal. A message that is a variable or a nested sprintf() is now flattened too, which is what left calls such as rlang::abort(unit_col, class = ...) spread over four lines. A call inside a `# nocov start` / `# nocov end` bracket is allowed a longer line, since collapsing it also removes the two marker lines and leaves a trailing `# nocov`; the limit is 175 characters there and 120 elsewhere. 44 calls collapsed and nocov brackets drop from 42 to 12. Whitespace is only collapsed outside string literals, so no message text changes. The 12 remaining brackets are not single calls (.onLoad, the lifecycle shims, a closure), wrap two statements, or would exceed 180 characters on one line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
No description provided.