extendr / rextendr

An R package that helps scaffolding extendr-enabled packages or compiling Rust code dynamically
https://extendr.github.io/rextendr/
Other
184 stars 27 forks source link

Exploring {cli} options #88

Closed Ilia-Kosenkov closed 3 years ago

Ilia-Kosenkov commented 3 years ago

In one of the latest PRs we brought in {cli}, which provides an exceptionally flexible tool for formatting messages. It supports {glue}-like syntax with inlined styles. It also provides CSS-like styles and themes, so the output, produced by {cli}, is highly configurable and does not depend on hard-coded constants in our code. Windows R still does not support UTF8 out of the box, so {cli} handles special symbol display, e.g. v vs . The styling is done through ANSI escape sequences, which are part of a standard character string, but are interpreted by a terminal as colors/font weights when printed out (as cat, not print).

image

The reason I created this issue is that I found no (simple) way to capture the output of {cli} functions as ansi formatted strings. What I would like to have is the following:

``` r cli::cli_alert_success("Function {.code package::function()} successfully wrote to file {.file dir/my_file.dat}.") #> [1] "\033[32mv\033[39m Function \033[38;5;235m\033[48;5;253m\033[38;5;235m\033[48;5;253m`package::function()`\033[48;5;253m\033[38;5;235m\033[49m\033[39m successfully wrote to file \033[34m\033[34mdir/my_file.dat\033[34m\033[39m.\n" ``` Created on 2021-03-16 by the [reprex package](https://reprex.tidyverse.org) (v1.0.0)

This is plain text, which can be manipulated, most importantly concatenated, and passed to stop() to provide nicely styled error messages. And the style of these messages matches that of regular cli::cli_* functions.

So, when using rextendr::ui_throw, we could potentially have something like this:

ui_throw(
    "An error occured in function {.fun package:function}.", 
    cli::cli_alert_danger("This bad thing has happened."),
    cli::cli_alert_danger("That bad thing happened to {.file my_file}."),
    cli::cli_alert_info("Please ensure that {.val {21 * 2}} == {.val {22 + 20}}."),
    "Plain text note"
)

image

This is technically possible, but requires some unorthodox methods, hijacking the {cli} output generation. I doubt we can allow such a thing in {rextendr} (though it requires just two short methods and some quasi-quotation in ui_throw), so I wonder if there is something we can use to replace {cli}.

clauswilke commented 3 years ago

I think we have to be careful about mission creep and adding tons of supporting code just to do a little bit of styling etc. If {usethis} can do what you want to do then just go ahead and use that, and otherwise I'd say see if you can make a PR to {cli} to add the features you need. I'm always wary of working around the limitations of existing packages. That's almost always the wrong approach. Better to fix the limitations upstream.

andy-thomason commented 3 years ago

Keeping dependencies small should be a priority for build times and future maintainability if possible.

On Tue, Mar 16, 2021 at 9:20 PM Claus Wilke @.***> wrote:

I think we have to be careful about mission creep and adding tons of supporting code just to do a little bit of styling etc. If {usethis} can do what you want to do then just go ahead and use that, and otherwise I'd say see if you can make a PR to {cli} to add the features you need. I'm always wary of working around the limitations of existing packages. That's almost always the wrong approach. Better to fix the limitations upstream.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/extendr/rextendr/issues/88#issuecomment-800613747, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL36XD542EZKFGSJIO5ETDTD7DR3ANCNFSM4ZJJ7KFA .

clauswilke commented 3 years ago

Keeping dependencies small should be a priority for build times and future maintainability if possible.

The constraints on an R package are somewhat different. The number of dependencies doesn't meaningfully affect build time. It can affect installation time though, in particular for continuous integration. So using a dependency is usually better than writing your own code that does the same thing. But, at the same time, you don't want to burden your users with having to install hundreds of packages.

Where it becomes a major problem in the R world is if the dependencies change their API frequently, so you constantly have to fix things that break, or if they are poorly maintained and may get pulled from CRAN. Neither is likely a problem for {usethis}.

malcolmbarrett commented 3 years ago

FWIW, I think usethis, as a mature package for dev workflows, is a reasonable and safe dependency for this project. I will say, though, that currently, usethis may introduce some CI complexity because of its dependency on gert and gert's dependency on libgit2. However, that seems to be improving and may not be much of an issue anyway for this project

Ilia-Kosenkov commented 3 years ago

The problem with {cli} is that it is confusing sometimes. We can write cli_alert_info("Writing to {.file file}"), but we cannot write a function ourselves that supports the same interpolation mechanism. I saw a few cases where others were invoking ui_throw, written by us, with such {cli} syntax, and it does not work. And if {cli} had an option to just return ansi string, we could have solved all the problems and had nice and consistent syntax. However, this is not a bug in {cli} it is its feature by design, so it cannot be fixed with a PR.

malcolmbarrett commented 3 years ago

Hold up, this just occurred to me: currently, rextendr depends on devtools. devtools depends on usethis, so if you're using that package, you already have an indirect dep

edit (sorry, this is more related to #68 than this issue at this point): by using devtools, you also indirectly depend on fs: you depend on usethis which depends on fs. devtools will also likely use fs in the future

Ilia-Kosenkov commented 3 years ago

That is actually true, I did not dig deep enough to find these dependencies.

``` r sort(tools::package_dependencies("devtools", recursive = TRUE)$devtools) #> [1] "askpass" "assertthat" "base64enc" "BH" "brew" #> [6] "brio" "cachem" "callr" "cli" "clipr" #> [11] "commonmark" "covr" "crayon" "credentials" "crosstalk" #> [16] "curl" "desc" "diffobj" "digest" "DT" #> [21] "ellipsis" "evaluate" "fansi" "fastmap" "fs" #> [26] "gert" "gh" "gitcreds" "glue" "graphics" #> [31] "grDevices" "highr" "htmltools" "htmlwidgets" "httr" #> [36] "ini" "jsonlite" "knitr" "later" "lazyeval" #> [41] "lifecycle" "magrittr" "markdown" "memoise" "methods" #> [46] "mime" "openssl" "pillar" "pkgbuild" "pkgconfig" #> [51] "pkgload" "praise" "prettyunits" "processx" "promises" #> [56] "ps" "purrr" "R6" "rappdirs" "rcmdcheck" #> [61] "Rcpp" "rematch2" "remotes" "rex" "rlang" #> [66] "roxygen2" "rprojroot" "rstudioapi" "rversions" "sessioninfo" #> [71] "stats" "stringi" "stringr" "sys" "testthat" #> [76] "tibble" "tools" "usethis" "utf8" "utils" #> [81] "vctrs" "waldo" "whisker" "withr" "xfun" #> [86] "xml2" "xopen" "yaml" "zip" ``` Created on 2021-03-19 by the [reprex package](https://reprex.tidyverse.org) (v1.0.0)

Unfortunately, this means we should consider throwing away {devtools}, because {roxygen2} has fewer dependencies:

``` r sort(tools::package_dependencies("roxygen2", recursive = TRUE)$roxygen2) #> [1] "assertthat" "brew" "callr" "cli" "commonmark" #> [6] "crayon" "desc" "digest" "evaluate" "glue" #> [11] "graphics" "grDevices" "highr" "knitr" "magrittr" #> [16] "markdown" "methods" "mime" "pkgbuild" "pkgload" #> [21] "prettyunits" "processx" "ps" "purrr" "R6" #> [26] "Rcpp" "rlang" "rprojroot" "rstudioapi" "stats" #> [31] "stringi" "stringr" "tools" "utils" "withr" #> [36] "xfun" "xml2" "yaml" ``` Created on 2021-03-19 by the [reprex package](https://reprex.tidyverse.org) (v1.0.0)

{devtools} is used in rextendr::document() and in test setup, which internally calls {usethis}.

Ilia-Kosenkov commented 3 years ago

I just realized I did not pay enough attention to {cli}'s docs. There apparently does exist a function that outputs raw formatted string that can be easily recycled: cli::cli_format_method(). However, from its description I got the impression that it is designed to provide format() method to custom types, so I simply ignored it. Bu in this issue https://github.com/r-lib/cli/issues/152 it is shown that it can be used to capture output, exactly what we need. I will make a PR to introduce this feature into our ui_throw() and friends.

If there are no other comments, this issue can be closed with that PR.

clauswilke commented 3 years ago

Yes, getting {devtools} out of the Imports section in DESCRIPTION would be good. It can (and has to) remain in Suggests.

Ilia-Kosenkov commented 3 years ago

@clauswilke, could you make a separate issue for {devtools} topic? This issue will be closed when we merge #89.