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

Implement {cli} to build better messages/warning/errors + use testthat3e and snapshot tests #425

Merged
merged 60 commits into from
Jul 8, 2021

Conversation

njtierney
Copy link
Collaborator

Resolves #418

@njtierney
Copy link
Collaborator Author

njtierney commented Jun 7, 2021

This takes care of some of the issues in #378

@njtierney
Copy link
Collaborator Author

Note that the first sentence in format_error is the header and subsequent vectors are new lines.

function_name <- "binomial"
msg <- cli::format_error(
  # first sentence is the header in bold
  c("Wrong function name provided in another model",
    # subsequent sentences are new lines
    "It looks like you're using {.pkg greta}'s {.fun {function_name}} function in \\
    the family argument of another model.",
    "Maybe you want to use {.code family = stats::{function_name}},instead?"
  )
)
stop(msg, call. = FALSE)
#> Error: Wrong function name provided in another model
#> It looks like you're using greta's `binomial()` function in the family argument
#> of another model.
#> Maybe you want to use `family = stats::binomial`,instead?

Created on 2021-06-07 by the reprex package (v2.0.0)

Session info
sessioninfo::session_info()
#> ─ Session info ───────────────────────────────────────────────────────────────
#>  setting  value                       
#>  version  R version 4.1.0 (2021-05-18)
#>  os       macOS Big Sur 10.16         
#>  system   x86_64, darwin17.0          
#>  ui       X11                         
#>  language (EN)                        
#>  collate  en_AU.UTF-8                 
#>  ctype    en_AU.UTF-8                 
#>  tz       Australia/Perth             
#>  date     2021-06-07                  
#> 
#> ─ Packages ───────────────────────────────────────────────────────────────────
#>  package     * version date       lib source        
#>  backports     1.2.1   2020-12-09 [1] CRAN (R 4.1.0)
#>  cli           2.5.0   2021-04-26 [1] CRAN (R 4.1.0)
#>  crayon        1.4.1   2021-02-08 [1] CRAN (R 4.1.0)
#>  digest        0.6.27  2020-10-24 [1] CRAN (R 4.1.0)
#>  ellipsis      0.3.2   2021-04-29 [1] CRAN (R 4.1.0)
#>  evaluate      0.14    2019-05-28 [1] CRAN (R 4.1.0)
#>  fansi         0.5.0   2021-05-25 [1] CRAN (R 4.1.0)
#>  fs            1.5.0   2020-07-31 [1] CRAN (R 4.1.0)
#>  glue          1.4.2   2020-08-27 [1] CRAN (R 4.1.0)
#>  highr         0.9     2021-04-16 [1] CRAN (R 4.1.0)
#>  htmltools     0.5.1.1 2021-01-22 [1] CRAN (R 4.1.0)
#>  knitr         1.33    2021-04-24 [1] CRAN (R 4.1.0)
#>  lifecycle     1.0.0   2021-02-15 [1] CRAN (R 4.1.0)
#>  magrittr      2.0.1   2020-11-17 [1] CRAN (R 4.1.0)
#>  pillar        1.6.1   2021-05-16 [1] CRAN (R 4.1.0)
#>  pkgconfig     2.0.3   2019-09-22 [1] CRAN (R 4.1.0)
#>  purrr         0.3.4   2020-04-17 [1] CRAN (R 4.1.0)
#>  reprex        2.0.0   2021-04-02 [1] CRAN (R 4.1.0)
#>  rlang         0.4.11  2021-04-30 [1] CRAN (R 4.1.0)
#>  rmarkdown     2.8     2021-05-07 [1] CRAN (R 4.1.0)
#>  rstudioapi    0.13    2020-11-12 [1] CRAN (R 4.1.0)
#>  sessioninfo   1.1.1   2018-11-05 [1] CRAN (R 4.1.0)
#>  stringi       1.6.2   2021-05-17 [1] CRAN (R 4.1.0)
#>  stringr       1.4.0   2019-02-10 [1] CRAN (R 4.1.0)
#>  styler        1.4.1   2021-03-30 [1] CRAN (R 4.1.0)
#>  tibble        3.1.2   2021-05-16 [1] CRAN (R 4.1.0)
#>  utf8          1.2.1   2021-03-12 [1] CRAN (R 4.1.0)
#>  vctrs         0.3.8   2021-04-29 [1] CRAN (R 4.1.0)
#>  withr         2.4.2   2021-04-18 [1] CRAN (R 4.1.0)
#>  xfun          0.23    2021-05-15 [1] CRAN (R 4.1.0)
#>  yaml          2.2.1   2020-02-01 [1] CRAN (R 4.1.0)
#> 
#> [1] /Library/Frameworks/R.framework/Versions/4.1/Resources/library

image

This is now starting to cover #378, just wort noting.

It will prepare things for #375 nicely, but I think that deserves it's own branch, due to the special error classes. #375 could also serve as an additional time to review the error messages created in this branch (#425)

@njtierney
Copy link
Collaborator Author

I'm unsure how to handle the ngettext function calls which are sometimes used when building an error message

@njtierney
Copy link
Collaborator Author

Due to the implementation of {cli}, the decision to use a pluralised sentence/word it needs to have the thing that is plural before the pluralisation decision point.

What this means is that if we want to use {cli} to build the sentence we need to have:

running 1 sampler in parallel, each on 2 cores or fewer

Compared to:

running 2 samplers in parallel, each on up to 2 cores

@goldingn do you have a preference for the sentence?

Here are the two implementations, the {cli} one is a bit more compact/concise and uses interpolated strings.

# old print

print_sample_core_info <- function(n_cores,
                                    samplers){
    cores_text <- ifelse(n_cores == 1,
                         "1 core",
                         sprintf("up to %i cores", n_cores)
    )
    
    msg <- sprintf(
      "\nrunning %i samplers in parallel, each on %s\n\n",
      length(samplers),
      cores_text
    )
    cli::cli_alert_info(msg)
}

print_sample_core_info(n_cores = 1, samplers = 1)
#> ℹ running 1 samplers in parallel, each on 1 core
print_sample_core_info(n_cores = 1, samplers = 1:2)
#> ℹ running 2 samplers in parallel, each on 1 core
print_sample_core_info(n_cores = 2, samplers = 1)
#> ℹ running 1 samplers in parallel, each on up to 2 cores
print_sample_core_info(n_cores = 2, samplers = 1:2)
#> ℹ running 2 samplers in parallel, each on up to 2 cores

# new method
print_sample_core_info2 <- function(n_cores = 1,
                                   samplers = 1:2){
    msg <- cli::format_message(
      "running {length(samplers)} sampler{?s} in parallel, each on \\
      {n_cores} core{?/s or fewer}",
    )
    cli::cli_alert_info(msg)
}

print_sample_core_info2(n_cores = 1, samplers = 1)
#> ℹ running 1 sampler in parallel, each on 1 core
print_sample_core_info2(n_cores = 1, samplers = 1:2)
#> ℹ running 2 samplers in parallel, each on 1 core
print_sample_core_info2(n_cores = 2, samplers = 1)
#> ℹ running 1 sampler in parallel, each on 2 cores or fewer
print_sample_core_info2(n_cores = 2, samplers = 1:2)
#> ℹ running 2 samplers in parallel, each on 2 cores or fewer

Created on 2021-06-08 by the reprex package (v2.0.0)

Session info
sessioninfo::session_info()
#> ─ Session info ───────────────────────────────────────────────────────────────
#>  setting  value                       
#>  version  R version 4.1.0 (2021-05-18)
#>  os       macOS Big Sur 10.16         
#>  system   x86_64, darwin17.0          
#>  ui       X11                         
#>  language (EN)                        
#>  collate  en_AU.UTF-8                 
#>  ctype    en_AU.UTF-8                 
#>  tz       Australia/Perth             
#>  date     2021-06-08                  
#> 
#> ─ Packages ───────────────────────────────────────────────────────────────────
#>  package     * version date       lib source        
#>  backports     1.2.1   2020-12-09 [1] CRAN (R 4.1.0)
#>  cli           2.5.0   2021-04-26 [1] CRAN (R 4.1.0)
#>  crayon        1.4.1   2021-02-08 [1] CRAN (R 4.1.0)
#>  digest        0.6.27  2020-10-24 [1] CRAN (R 4.1.0)
#>  ellipsis      0.3.2   2021-04-29 [1] CRAN (R 4.1.0)
#>  evaluate      0.14    2019-05-28 [1] CRAN (R 4.1.0)
#>  fansi         0.5.0   2021-05-25 [1] CRAN (R 4.1.0)
#>  fs            1.5.0   2020-07-31 [1] CRAN (R 4.1.0)
#>  glue          1.4.2   2020-08-27 [1] CRAN (R 4.1.0)
#>  highr         0.9     2021-04-16 [1] CRAN (R 4.1.0)
#>  htmltools     0.5.1.1 2021-01-22 [1] CRAN (R 4.1.0)
#>  knitr         1.33    2021-04-24 [1] CRAN (R 4.1.0)
#>  lifecycle     1.0.0   2021-02-15 [1] CRAN (R 4.1.0)
#>  magrittr      2.0.1   2020-11-17 [1] CRAN (R 4.1.0)
#>  pillar        1.6.1   2021-05-16 [1] CRAN (R 4.1.0)
#>  pkgconfig     2.0.3   2019-09-22 [1] CRAN (R 4.1.0)
#>  purrr         0.3.4   2020-04-17 [1] CRAN (R 4.1.0)
#>  reprex        2.0.0   2021-04-02 [1] CRAN (R 4.1.0)
#>  rlang         0.4.11  2021-04-30 [1] CRAN (R 4.1.0)
#>  rmarkdown     2.8     2021-05-07 [1] CRAN (R 4.1.0)
#>  rstudioapi    0.13    2020-11-12 [1] CRAN (R 4.1.0)
#>  sessioninfo   1.1.1   2018-11-05 [1] CRAN (R 4.1.0)
#>  stringi       1.6.2   2021-05-17 [1] CRAN (R 4.1.0)
#>  stringr       1.4.0   2019-02-10 [1] CRAN (R 4.1.0)
#>  styler        1.4.1   2021-03-30 [1] CRAN (R 4.1.0)
#>  tibble        3.1.2   2021-05-16 [1] CRAN (R 4.1.0)
#>  utf8          1.2.1   2021-03-12 [1] CRAN (R 4.1.0)
#>  vctrs         0.3.8   2021-04-29 [1] CRAN (R 4.1.0)
#>  withr         2.4.2   2021-04-18 [1] CRAN (R 4.1.0)
#>  xfun          0.23    2021-05-15 [1] CRAN (R 4.1.0)
#>  yaml          2.2.1   2020-02-01 [1] CRAN (R 4.1.0)
#> 
#> [1] /Library/Frameworks/R.framework/Versions/4.1/Resources/library

@goldingn
Copy link
Member

goldingn commented Jun 9, 2021

Message formatting aside, does it make sense to report this using cli alerts? This bit isn't a CLI, as everything only affects objects within an R session rather than affecting the system. I think having messages formatted as cli alerts will confuse people.

Is this formatting restriction the same in glue? Or when using ngettext()?

@njtierney
Copy link
Collaborator Author

Message formatting aside, does it make sense to report this using cli alerts? This bit isn't a CLI, as everything only affects objects within an R session rather than affecting the system. I think having messages formatted as cli alerts will confuse people.

My understanding was that the cli alert was giving nice icons (like the tick, cross, "i" symbol for warning or general alert), combined with glue string interpolation, pluralisation, and other niceties, like specially formatting error messages, providing classes for data objects that are functions, classes, and turning vectors into lists - e.g.,

vec <- c("pkg1", "pkg2", "pkg3")
cli::cli_alert_info("The following packages were installed: {vec}")
#> ℹ The following packages were installed: pkg1, pkg2, and pkg3

I'm not sure using cli will make things confusing, but it could make them appear inconsistent in terms of text and styling if mixed with other messages that don't have similar formatting. In this case I would consider it a helpful alert that would generally appear as white text - "hey, here's some info about the samplers/cores/chains".

That said, the progress bar would be generally rendered in orangeish text, and the message above would be in white, so it might look a bit strange?

Is this formatting restriction the same in glue? Or when using ngettext()?

Glue only does string interpolation, and does not do pluralisation, that is only done in cli

I think I completely misunderstood ngettext 😅 - I read "translation" as allowing the text to get translated to other languages (french, spanish, chinese, etc):

If Native Language Support (NLS) was enabled in this build of R (see the bindtextdomain() example), attempt to translate character vectors or set where the translations are to be found.

But reading it now it specifically helps with pluralisation. This can be implemented with cli

@njtierney
Copy link
Collaborator Author

This PR is now ready for a review (of error messages).

I've given the error messages a clean up and clarified where I can, but I still think that there is scope for a further review of the error messages, perhaps to be in line with suggestions on https://style.tidyverse.org/error-messages.html

This could be implemented in another PR, or we can spend some time reviewing the messages now and then also close #382 .

A lot of the tests for greta will now fail, as they are checking the error/warning messages, which have changed. Fixing these could be tedious, if it is quite tedious, we could change the tests over to use snapshot tests, in Testthat 3e. However, that would involve also addressing issues with using mock (#381). So things are a bit interconnected at the moment.

tests/testthat/_snaps/as_data.md Show resolved Hide resolved
tests/testthat/_snaps/calculate.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/calculate.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/calculate.md Show resolved Hide resolved
tests/testthat/_snaps/calculate.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/operators.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/simulate.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/syntax.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/variables.md Show resolved Hide resolved
tests/testthat/_snaps/variables.md Outdated Show resolved Hide resolved
@njtierney njtierney merged commit d362c65 into greta-dev:master Jul 8, 2021
@njtierney njtierney deleted the implement-cli-issue-418 branch July 8, 2021 03:37
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.

Use {cli} for messages and prompts
2 participants