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.

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

Standardize / clarify error / warning messages #397

Open
pearsonca opened this issue Nov 23, 2023 · 0 comments
Open

Standardize / clarify error / warning messages #397

pearsonca opened this issue Nov 23, 2023 · 0 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed

Comments

@pearsonca
Copy link
Collaborator

pearsonca commented Nov 23, 2023

As highlighted during the review of #376, our warning / error / messaging content could use work.

Propose:

  • draft an update to STYLE_GUIDE.md covering the desired format of such messages AND where we should vs should not provide them (e.g. because the base errors are sufficient and doing our own version is simply building future maintenance burden).
  • make that a draft PR
  • then comprehensive review of all warning / error / messages to meet the STYLE_GUIDE.md target.

Example issues with current approach:

Calling Scope / Insufficient Feedback

check_group_date_unique <- function(obs) {
  group_cols <- c("reference_date", "report_date", ".group")
  obs <- coerce_dt(obs, required_cols = group_cols, copy = FALSE)
  cells <- obs[, .(count = .N), by = group_cols]
  if (any(cells[, count > 1])) {
    rlang::abort(
      paste0(
        "The input data seems to be stratified by more variables ",
        "than specified via the `by` argument. Please provide additional ",
        "grouping variables to `by`, ",
        "or aggregate the observations beforehand."
      )
    )
  }
  return(invisible(NULL))
}

This message might be fine in practice, but it's a bit tough to tell without looking around at everywhere its used and trying it in situ. Perhaps this check needs to pass through the grouping columns as well? I'm generally in favor of explicitly showing the logic of how a requirement failed + particular values at failure.

Explicitly Refer to Arguments

# clipped from `check_module`
 if (!"data" %in% names(module)) {
    rlang::abort(
      "Must contain a list component specifying the data requirements for
       further modelling as a list"
    )
  }
  if (!is.list(module[["data"]])) {
    rlang::abort(
      "data must be a list of required data"
    )
  }

In general, we should explicitly refer to the argument with problems (in this , case module - though if we can get at the call stack and use whatever variable name the user passed to that argument, that would be ideal). If there are sub-elements that we're checking, we should be indicating the subelement connection (in this case, refer to module[["data"]] instead of "data").

Base R Errors

rw <- function(time, by, type = c("independent", "dependent")) {
  type <- match.arg(type)
  if (missing(time)) {
    rlang::abort("time must be present")
  } else {
    time <- deparse(substitute(time))
  }

Do we need a special handling case for something R is just going to complain about?

@pearsonca pearsonca added documentation Improvements or additions to documentation help wanted Extra attention is needed good first issue Good for newcomers labels Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant