Validate chart_type early and improve the error message#217
Validate chart_type early and improve the error message#217HorridTom merged 8 commits intoHorridTom:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks @ahmadmkhatib this looks good. A couple of suggestions:
I don't think these changes should take much work from what you have already, but let me know if anything not clear or if I can help in any way. |
|
Yes @HorridTom, agree and will work on these suggestions |
|
@HorridTom, I added the function in an R file now called argument_validation. Check it out. |
| # Length check | ||
| if (length(chart_type) != 1) { | ||
| stop( | ||
| "chart_type must be a single value. ", |
There was a problem hiding this comment.
I think rather than "must be a single value" let's have "must have length one."
| #' | ||
| #' @return Invisibly returns TRUE if valid; otherwise errors. | ||
| #' | ||
| #' @keywords internal |
There was a problem hiding this comment.
I think with @noRd, @keywords internal is not required? But could you check?
| #' 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. |
There was a problem hiding this comment.
As above re. "single value" vs "length one", plus missing space before "Must"
Great, thanks @ahmadmkhatib - just a couple of minor comments now. Plus you might want to add a few more tests to cover the other checks you've added? (i.e. chart_type |
|
Hi @HorridTom - I enhanced the presentation and added more tests |
1) added @nord back into roxygen skeleton for validate_chart_type() - since this is an internal only function there should not be a .Rd man file for this 2) There was already a check for chart_type NULL in preprocess_inputs(), with a deprecation warning. I have moved this into validate_chart_type() and elevated from warning to error 3) Adjusted test to account for this change (2) 4) Added a missing space in the error message for length != 1
|
Hi @ahmadmkhatib - this looks great now. I made a few tweaks, see c8fb2bc above. If you're happy with these I'll go ahead and merge. |
|
Great, yes, happy, let's merge |
2 participants
I added early validation for chart_type in autospc(), replacing the old error with a clear, user-facing message listing supported chart types.