Skip to content

Convert error, warning, and message calls to cli messaging - #800

Open
alanahjonas95 wants to merge 24 commits into
mainfrom
aj641247-gsk-cli-messaging-conversion
Open

Convert error, warning, and message calls to cli messaging#800
alanahjonas95 wants to merge 24 commits into
mainfrom
aj641247-gsk-cli-messaging-conversion

Conversation

@alanahjonas95

@alanahjonas95 alanahjonas95 commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

resolves #748

alanahjonas95 and others added 16 commits August 10, 2026 16:45
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Escape literal braces in cli suggestions and align tests/snapshots with the cli message formatting shown in CI logs. Also replace deparse()+paste() with deparse1() to satisfy lintr.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Align the remaining apply_tfrmt/page_plan expectations with the actual cli message formatting seen in CI, and restore the big_n warning text so duplicated columns remain readable in snapshots.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cli::cli_warn() joins unnamed multi-element message vectors into a
single wrapped line rather than preserving them as separate lines,
so update the big_n snapshot to reflect the actual output.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread R/apply_footnote_meta.R Outdated
}

message(paste(message_text, collapse = "\n"))
cli::cli_inform(paste(message_text, collapse = "\n"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure the paste() call is needed here. The formatting seems to be off a bit (no space between footnote and "Test footnote 1" and the line break after for the).

We could get a bit more sophisticated and build the message (or the message details) with bullets:

cli::cli_inform(
    c(
        "The provided column location does not exist in the provided data for the footnote\"Test footnote 1\"",
        "i" = "Provided column location:",
        "*" = "trt: `Not A Column`"

    )
)
#> The provided column location does not exist in the provided data for the
#> footnote"Test footnote 1"
#> ℹ Provided column location:
#> • trt: `Not A Column`

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

agreed, this is nicer. I've updated the message using cli_inform() with an "i" info line and dynamically generated "*" bullets for each column/value pair

Comment thread R/apply_frmt_methods.R Outdated

if (length(miss_param_from_data) > 0) {
stop(paste0(
cli::cli_abort(paste0(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

{cli} supports string interpolation with {glue}. Looking into this I realised this stop() / cli::cli_abort() call is not covered by unit tests.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Converted this to cli/glue implicit interpolation and added a new unit test

Comment thread R/apply_frmt_methods.R Outdated
id_cols <- .tmp_data %>%
dplyr::select(!!!column, !!label, !!!group, !!param)
warning(paste0(
cli::cli_warn(paste0(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cli + paste or paste0 can usually be substituted with cli + implicit glue interpolation

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

replaced the paste/paste0 concatenation with implicit glue interpolation. Also swept the rest of the package for any other cli + paste/paste0 call sites and converted those to implicit glue interpolation too.

alanahjonas95 and others added 7 commits August 11, 2026 10:07
- apply_footnote_meta.R: rebuild the column-location-not-found message
  using cli::cli_inform() with proper spacing and bullet points instead
  of a manually paste()-joined string.
- apply_frmt_methods.R: replace paste0()/toString() concatenation in the
  missing-parameter cli_abort() and uniqueness cli_warn() calls with
  cli/glue implicit string interpolation.
- Add a unit test covering the previously-untested 'parameters missing
  from the data' error path in apply_frmt.frmt_combine().

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…olation

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@alanahjonas95
alanahjonas95 requested a review from dragosmg August 11, 2026 12:15
@github-actions

Copy link
Copy Markdown
Contributor

🦺 Coverage summary

badge

🟢 Merging PR #800 (38e367f) into main (c9c78d9) will increase overall coverage by 0.1 percentage points.
🔴 Diff coverage is 84.7% (182 out of 215 modified lines are covered by tests). It's good practice to aim for at least 97% (the base branch test coverage).

Details

Files with changes in coverage

File name Coverage head Coverage base Δ
R/apply_frmt_methods.R 97.7% 95.8% 1.9 ⬆️
R/body_plan.R 94.7% 92.8% 1.9 ⬆️
R/col_plan.R 92.3% 93.5% -1.2 ⬇️
R/col_style_plan.R 93.9% 94.1% -0.2 ⬇️
R/footnote_plan.R 95.7% 95.4% 0.3 ⬆️
R/print_to_ggplot.R 87.2% 86.8% 0.4 ⬆️
R/row_group_plan.R 73.1% 67.9% 5.2 ⬆️
R/tfrmt_sigdig.R 93.2% 94.4% -1.2 ⬇️
R/tfrmt.R 96.2% 96.5% -0.3 ⬇️
Overall 97.1% 97% 0.1 ⬆️

Coverage for modified lines

File name Lines modified Lines tested Coverage Missing
R/JSON.R 4 1 25% 17, 76, 214
R/apply_col_style_plan.R 3 3 100%
R/apply_footnote_meta.R 19 19 100%
R/apply_frmt_methods.R 18 17 94.4% 178
R/apply_page_plan.R 3 3 100%
R/apply_row_grp_plan.R 4 4 100%
R/apply_table_frmt_plan.R 1 1 100%
R/apply_tfrmt.R 27 27 100%
R/big_n.R 6 6 100%
R/body_plan.R 3 0 0% 37-39
R/col_plan.R 11 10 90.9% 245
R/col_style_plan.R 10 9 90% 129
R/display_insights.R 3 3 100%
R/extract_data.R 1 1 100%
R/footnote_plan.R 6 5 83.3% 124
R/frmt_plans.R 6 0 0% 50, 62, 68, 70, 212, 214
R/page_plan.R 2 0 0% 113, 115
R/print_to_ggplot.R 14 13 92.9% 55
R/print_to_gt.R 10 10 100%
R/row_group_plan.R 6 0 0% 46, 47, 49, 86, 92, 94
R/struct_utils.R 4 4 100%
R/tfrmt.R 10 8 80% 263, 345
R/tfrmt_checks.R 24 24 100%
R/tfrmt_layer.R 8 7 87.5% 182
R/tfrmt_n_pct.R 2 2 100%
R/tfrmt_sigdig.R 10 5 50% 63, 225, 233-235
Total 215 182 84.7%

♻️ Comment updated with the latest results.

Created on 2026-08-11 with covr2gh v0.0.0.9041.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harmonise condition handling and messaging

2 participants