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

Use cli #1956

Merged
merged 111 commits into from
Mar 4, 2024
Merged

Use cli #1956

merged 111 commits into from
Mar 4, 2024

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Feb 29, 2024

This is a huge PR, most of which is mind-numbingly repetitive. Recommended way to review:

  • Read the new developer-facing article vignettes/articles/ui-cli-conversion.Rmd, which is glorified notes-to-self. I may create a blog post about this and these notes would be helpful. This will also be helpful to link to in future PRs that introduce unconventional UI. It is probably worth doing pr_fetch(1956) and rendering this because then you can enjoy the asciicast chunks that really show the before and after.
  • Look for the comments I sprinkled around, where I'll draw attention to a few interesting things.
  • utils-ui.R is the most important file, because that's where the new UI functions live.

I plan to merge this soon, but am happy to receive feedback. But I really want to get friends and family using this soon so we can put a few miles on it be fore I submit to CRAN.

Closes #956, closes #1809

@@ -15,13 +15,15 @@ use_addin <- function(addin = "new_addin", open = rlang::is_interactive()) {
if (!file_exists(addin_dcf_path)) {
create_directory(proj_path("inst", "rstudio"))
file_create(addin_dcf_path)
ui_done("Creating {ui_path(addin_dcf_path)}")
ui_bullets(c("v" = "Creating {.path {pth(addin_dcf_path)}}"))
Copy link
Member Author

@jennybc jennybc Feb 29, 2024

Choose a reason for hiding this comment

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

ui_bullets() is a new unexported wrapper around cli::cli_bullets() that applies a custom usethis style and honors the usethis.quiet option. All of the old "block styles" get replaced by ui_bullets().

{.path {pth(addin_dcf_path)}} is a clunky but effective pattern you will see a lot of. This is explained more fully in the article.

pth() is an internal helper that forms paths relative to a base and it's super important in usethis. But I also really, really want cli's file hyperlinks. I don't see a way to create a custom inline span that allows me to combine my pre-processsing with cli's file hyperlink support. But I'd love to learn that I'm wrong.

Comment on lines +587 to +588
# TODO: the ui_paths used to be a nested bullet list
# if that ever becomes possible/easier with cli, go back to that
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to have nested bullets in cli::cli_bullets(). But AFAICT that's only possible if you're making a bullet list more "by hand" with cli_ol(), cli_ul(), cli_li(), and the like.

ui_paths <- map_chr(paths, ui_path_impl)
ui_bullets(c(
"i" = "{cli::qty(n)}There {?is/are} {n} untracked file{?s}:",
bulletize(usethis_map_cli(ui_paths, template = "{.file <<x>>}"))
Copy link
Member Author

Choose a reason for hiding this comment

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

In several places I need to programmatically make bullet lists and, at the same time, apply inline styling. usethis_map_cli() and bulletize() are versions of similar functions I've written in, e.g., googledrive.

kv_line("Global (user-level) gitignore file", git_ignore_path("user"))
kv_line(
"Global (user-level) gitignore file",
I("{.path {git_ignore_path('user')}}")
Copy link
Member Author

Choose a reason for hiding this comment

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

Occasionally the value of a kev-value line (which is what kv_line() handles) needs explicit inline styling and interpolation. Applying AsIs allows to me detect that. The default behaviour is to style the value as {.val {value}}.

DESCRIPTION.",
" " = "usethis only supports modification of the {.field Authors@R} field.",
"i" = "We recommend one of these paths forward:",
"_" = "Delete the legacy fields and rebuild with {.fun use_author}; or",
Copy link
Member Author

Choose a reason for hiding this comment

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

I introduce a new bullet type with shortcode "_". This is for TODOs and is meant to evoke a place where you could check something off.

cli::cli_abort(
message,
class = c(class, "usethis_error"),
.envir = .envir,
...
)
}

# questions --------------------------------------------------------------------
ui_yep <- function(x,
Copy link
Member Author

Choose a reason for hiding this comment

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

Very similar to ui_yeah() but cli is used for preparing utils:menu(title =, choices =).

Error:
! User input required, but session is not interactive.
Query: Do you want to cancel this operation and sort that out first?
Error in `ui_yep()`:
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that usethis uses rlang::abort() indirectly, it becomes obvious that I also haven't yet threaded the caller through everywhere. That is a separate job.

* Delete these fields and rebuild with `use_author()`.
* Convert to Authors@R with `desc::desc_coerce_authors_at_r()`,
then delete the legacy fields.
[ ] Delete the legacy fields and rebuild with `use_author()`; or
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a little bit of a bummer that the TODO checkbox becomes [ ] when Unicode is not available, so it takes up more characters than the other bullets. But I really think we care more about making the output pretty in a Unicode setting.

@@ -9,18 +9,188 @@
[7] "fork_cannot_push_origin" "fork_upstream_is_not_origin_parent"
[9] "upstream_but_origin_is_not_fork"

# fork_upstream_is_not_origin_parent is detected
# 'no_github' is reported correctly
Copy link
Member Author

Choose a reason for hiding this comment

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

Snapshot tests that are newly possible.

@@ -0,0 +1,230 @@
cli::test_that_cli("ui_bullets() look as expected", {
Copy link
Member Author

Choose a reason for hiding this comment

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

cli::test_that_cli() is a supercool function! 🤓

@jennybc jennybc marked this pull request as ready for review March 1, 2024 00:48
I guess temp paths vary in length, even across separate runs on the same platform?
@jennybc jennybc mentioned this pull request Mar 1, 2024
Copy link
Contributor

@olivroy olivroy left a comment

Choose a reason for hiding this comment

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

Looks very good! Huge amount of work.

Just need to make sure .run, .fun .code, .help are used consistently.

.run should be used for things that you almost always want to run immediately after seeing it in the current session. (no base functions ideally to avoid bad UX in RStduio) Edit: IDE won't let you run anything with extra parenthesis inside
code for base fns cli::cli_text("{.run cli::cli_bullets(c('a', ''a))}") will show a forbidden tooltip.
help when wanting to direct to help file
fun elsewhere?

R/badge.R Show resolved Hide resolved
R/git-default-branch.R Outdated Show resolved Hide resolved
R/git-default-branch.R Outdated Show resolved Hide resolved
R/git.R Outdated Show resolved Hide resolved
R/git.R Outdated Show resolved Hide resolved
R/sitrep.R Outdated Show resolved Hide resolved
R/sitrep.R Outdated Show resolved Hide resolved
R/spelling.R Show resolved Hide resolved
R/test.R Show resolved Hide resolved
R/tidyverse.R Show resolved Hide resolved
@jennybc
Copy link
Member Author

jennybc commented Mar 1, 2024

Thanks @olivroy! I know that my mojo and style around .run, .fun, .code, .help evolved over the course of this task and I didn't have the heart to go back and rationalize it.

I've accepted all the suggestions that I can. If you want to convert some of the remaining comments to suggestions, I would accept those too.

@olivroy
Copy link
Contributor

olivroy commented Mar 1, 2024

Thanks @olivroy! I know that my mojo and style around .run, .fun, .code, .help evolved over the course of this task and I didn't have the heart to go back and rationalize it.

I will send a follow-up post-merge instead (if necessary). My browser has difficulty handling this PR :)

I've accepted all the suggestions that I can. If you want to convert some of the remaining comments to suggestions, I would accept those too.

Great!

@jennybc jennybc mentioned this pull request Mar 1, 2024
7 tasks
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I took a quick look and it looks good to me! Feel free to ignore any comments that you don't are worth the effort; I mostly wanted to draw your attention to things that I spotted not specifically request changes.


Summary of what I've done for todo's:

- Introduce a new bullet shortcode for a todo. Proposal: `_` (instead of `*`), which seems to be the best single ascii character that evokes a place to check something off.
Copy link
Member

Choose a reason for hiding this comment

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

[] would another option. It's not a single character but it does better evoke the checkbox.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I care more about the shortcode being single character and, therefore, lining up nicely with its friends in code, than about evoking the checkbox.


Here's the general conversion plan:

- `ui_field()` becomes `{.field blah}`. In `usethis_theme()`, I tweak the `.field` style to apply single quotes if color is not available, which is what `ui_field()` has always done.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to do this in cli, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

))
```
It would be nice to create a custom inline class, e.g. `{.ui_path {some_path}}`, which I have done in, e.g., googledrive.
But it's not easy to do this *while still inheriting cli's `file:` hyperlink behaviour*, which is very desirable.
Copy link
Member

Choose a reason for hiding this comment

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

What if you used format_inline() + .file in your custom inline helper? Or do you think the usethis path logic is sufficiently useful that it should move to cli?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I'd rather not "explode" (as in pre-format) filepaths like this. It introduces a special behaviour that I suspect would come back to haunt me. In general, I try to keep the actual formatting as late as possible to reduce the chance of calling format_inline() under one set of conditions, but then having another set hold when the user actually sees the message.

Copy link
Member Author

Choose a reason for hiding this comment

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

In terms of having something similar in cli, I'm not sure how you would pass the base information to an inline style? I don't see how to pass 2 pieces of info to an inline style (display path + optional base to make it relative to).

}
```

The main point of `ui_abort()` is to use to `cli_abort()` (and to continue applying the `"usethis_error"` class).
Copy link
Member

Choose a reason for hiding this comment

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

I assume we mostly use that error class for tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.

Query: Do you want to cancel this operation and sort that out first?
Error in `ui_yep()`:
x User input required, but session is not interactive.
i Query: "Do you want to cancel this operation and sort that out first?"
Copy link
Member

Choose a reason for hiding this comment

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

Drop "query:"? I think the i bullet is now sufficient.

* Add badges in documentation topics by inserting one of:
v Adding lifecycle to 'Imports' field in DESCRIPTION.
[ ] Refer to functions with `lifecycle::fun()`.
v Adding "@importFrom lifecycle deprecated" to 'R/{TESTPKG}-package.R'.
Copy link
Member

Choose a reason for hiding this comment

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

Why the mix of double and single quotes?

Copy link
Member Author

@jennybc jennybc Mar 4, 2024

Choose a reason for hiding this comment

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

This comes from cli and it's a quoting difference between {.val VALUE} and {.file some/path/to/some/thing}.


span.val -> cli_format.character -> default is "

https://github.com/r-lib/cli/blob/1b5e691a44497c8a3708ed33ab2026c640a2c66c/R/themes.R#L238-L241

...
    span.val = list(
      transform = function(x, ...) cli_format(x, ...),
      color = "blue"
    ),
...

https://github.com/r-lib/cli/blob/1b5e691a44497c8a3708ed33ab2026c640a2c66c/R/format.R#L73

cli_format.character <- function(x, style = NULL, ...) {
  quote <- style$`string-quote` %||% style$string_quote %||% "\""
  encodeString(x, quote = quote)
}

span.file -> theme_file() -> quote_weird_name -> default is '

https://github.com/r-lib/cli/blob/1b5e691a44497c8a3708ed33ab2026c640a2c66c/R/themes.R#L207

...
span.file = theme_file(),
...

https://github.com/r-lib/cli/blob/1b5e691a44497c8a3708ed33ab2026c640a2c66c/R/themes.R#L365

theme_file <- function() {
  list(
    color = "blue",
    transform = function(x) make_link(x, type = "file"),
    fmt = quote_weird_name
  )
}

https://github.com/r-lib/cli/blob/1b5e691a44497c8a3708ed33ab2026c640a2c66c/R/themes.R#L294-L300

quote_weird_name <- function(x) {
  x2 <- quote_weird_name0(x)
  if (x2[[2]] || num_ansi_colors() == 1) {
    x2[[1]] <- paste0("'", x2[[1]], "'")
  }
  x2[[1]]
}

@jennybc jennybc merged commit fc8472c into main Mar 4, 2024
13 checks passed
@jennybc jennybc deleted the use-cli branch March 5, 2024 17:17
This was referenced Mar 6, 2024
@olivroy olivroy mentioned this pull request Mar 14, 2024
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.

ui_cli_inform() output looks different from other output Migrate ui_*() functionality to use cli
3 participants