-
Notifications
You must be signed in to change notification settings - Fork 1
Validate chart_type early and improve the error message #217
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
Changes from 4 commits
a6362cd
c8c77e0
6edfef8
ca309ad
9fa5f32
c8fb2bc
a889d58
945e200
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
|
|
||
| #' Validate chart_type argument | ||
| #' | ||
| #' Checks that `chart_type` is a single, non-NULL character string corresponding | ||
| #' to a supported SPC chart type. Intended for internal use only. | ||
| #' | ||
| #' @param chart_type Character scalar specifying chart type. | ||
| #' | ||
| #' @return Invisibly returns TRUE if valid; otherwise errors. | ||
| #' | ||
| #' @keywords internal | ||
| #' @noRd | ||
|
|
||
|
|
||
| validate_chart_type <- function(chart_type) { | ||
|
|
||
| allowed_chart_types <- c("XMR", "MR", "C", "C'", "P", "P'") | ||
|
|
||
| # NULL check | ||
| if (is.null(chart_type)) { | ||
| stop( | ||
| "chart_type must be provided. Available chart types are: ", | ||
| paste(allowed_chart_types, collapse = ", "), | ||
| ".", | ||
| call. = FALSE | ||
| ) | ||
| } | ||
|
|
||
| # Length check | ||
| if (length(chart_type) != 1) { | ||
| stop( | ||
| "chart_type must be a single value. ", | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think rather than "must be a single value" let's have "must have length one." |
||
| "Available chart types are: ", | ||
| paste(allowed_chart_types, collapse = ", "), | ||
| ".", | ||
| call. = FALSE | ||
| ) | ||
| } | ||
|
|
||
| # Type check (defensive) | ||
| if (!is.character(chart_type)) { | ||
| stop( | ||
| "chart_type must be a character string.", | ||
| call. = FALSE | ||
| ) | ||
| } | ||
|
|
||
| # Value check | ||
| if (!chart_type %in% allowed_chart_types) { | ||
| stop( | ||
| sprintf( | ||
| "Invalid chart_type: '%s'. Available chart types are: %s.", | ||
| chart_type, | ||
| paste(allowed_chart_types, collapse = ", ") | ||
| ), | ||
| call. = FALSE | ||
| ) | ||
| } | ||
|
|
||
| invisible(TRUE) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,8 @@ | |
| #' } | ||
| #' @param n Name of column (passed using tidyselect semantics) to use as | ||
| #' denominator for P and P' charts. | ||
| #' @param chart_type The type of chart you wish to plot. Available options are: | ||
| #' "XMR", "MR", "C", "C'", "P", "P'". | ||
| #' @param chart_type The type of chart you wish to plot.Must be a single value. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above re. "single value" vs "length one", plus missing space before "Must" |
||
| #' Available options are: "XMR", "MR", "C", "C'", "P", "P'". | ||
| #' | ||
| #' ## Algorithm Parameters | ||
| #' Parameters that control behaviour of the algorithm used to re-establish | ||
|
|
@@ -249,7 +249,8 @@ autospc <- function(data, | |
| upper_annotation_sf <- preprocessed_vars$upper_annotation_sf | ||
| lower_annotation_sf <- preprocessed_vars$lower_annotation_sf | ||
|
|
||
|
|
||
| validate_chart_type(chart_type) | ||
|
|
||
| # Get control limits | ||
| data <- create_SPC_auto_limits_table( | ||
| data, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| test_that("autospc errors informatively for invalid chart_type", { | ||
|
|
||
| expect_error( | ||
| autospc( | ||
| ed_attendances_monthly, | ||
| chart_type = "XmR'", | ||
| x = month_start, | ||
| y = att_all | ||
| ), | ||
| "Available chart types are" | ||
| ) | ||
|
|
||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with
@noRd,@keywords internalis not required? But could you check?