Introduce `cli_menu()` and friends by jennybc · Pull Request #242 · r-lib/gargle

jennybc · 2023-04-03T23:42:49Z

This is functionally important for gargle, because cli_menu() is ultimately implemented on top of readline(), not utils::menu(). And Jupyter has shimmed readline() to work even though, at the time of writing, base::interactive() returns FALSE in Jupyter.

Therefore this PR makes gargle's interactive choices work in Jupyter notebooks, which notably covers Google Colab.

This is part (but not all) of making user auth "just work" on Colab (#140).

I have adapted cli_menu() from https://github.com/rstudio/rsconnect/blob/main/R/utils-cli.R. I think most of my changes might be generally positive and could be considered when/if something like this goes into cli itself (r-lib/cli#228). I also added tests.

@hadley @gaborcsardi I don't really need feedback on this (though it's welcome). This is more of an FYI request for review, to continue the discussion around r-lib/cli#228. I'll make a few comments inline.

Links to the Jupyter + readline() story:

IRkernel/IRkernel#452

https://github.com/IRkernel/IRkernel/blob/1eddb304b246c14b62949abd946e8d4ca5080d25/R/execution.r#L131-L137

https://github.com/IRkernel/IRkernel/blob/1eddb304b246c14b62949abd946e8d4ca5080d25/R/execution.r#L271-272

jennybc · 2023-04-03T23:47:12Z

+                     prompt,
+                     choices,
+                     not_interactive = choices,
+                     exit = integer(),


I called this exit (instead of quit) to better match the typical vocabulary here (e.g. in the docs for utils::menu()) and to avoid any confusion with actuallly quitting R.

jennybc · 2023-04-03T23:48:34Z

+  # guard against invalid mocked input
+  local_input <- getOption("cli_input", character())


I was able to get into an infinite loop without this. Of course it only happens when one enters invalid mocked input, but still seemed worth accounting for.

jennybc · 2023-04-03T23:49:05Z

+    if (length(local_input) > 0) {
+      cli::cli_abort(
+        c(x = "Internal error: mocked input is invalid."),
+        .envir = .envir,
+        call = error_call
+      )
+    }


The second part of "avoiding an infinite loop".

Can we do that all in one place? Or extract it out into a helper?

OK I consolidated things, just to make it a bit neater, at least.

Why does it error if it’s length greater than zero? The original intent was to be able to supply multiple values that were used in turn.

Over in rsconnect, if I add this test, it causes the test suite to just hang:

test_that("cli_menu() works with multiple inputs", { simulate_user_input(3) expect_snapshot( cli_menu( "Let's talk", "Are you OK?", choices = c("Yes", "No") ) ) })

I guess I haven't found an organic need to supply multiple values. But I have managed to supply an invalid one. So it seems nice for development purposes to handle that case.

I can try to think about how to preserve both.

Are you convinced that multiple mocked inputs is important?

It feels like we could just make multiple calls to simulate_user_input() (rsconnect) or local_user_input() (here in gargle) in such a test and let the functions be simpler.

I realize I can pursue the simplification that's most natural here. I'm mostly talking about what we might eventually want in cli or in some standalone file for general use.

jennybc · 2023-04-03T23:49:33Z

+  selected <- as.integer(selected)
+  if (selected %in% c(0, exit)) {
+    if (is_testing()) {
+      cli::cli_abort("Exiting...", call = NULL)


Exiting not quitting

jennybc · 2023-04-03T23:49:39Z

+    if (is_testing()) {
+      cli::cli_abort("Exiting...", call = NULL)
+    } else {
+      cli::cli_alert_danger("Exiting...")


jennybc · 2023-04-03T23:50:17Z

+    } else {
+      cli::cli_alert_danger("Exiting...")
+      # simulate user pressing Ctrl + C
+      invokeRestart("abort")


Omitted the rogue cnd that the rsconnect version has here (presumably a think-o).

jennybc · 2023-04-03T23:51:07Z

+  }
+}
+
+local_user_input <- function(x, env = caller_env()) {


Went with a local_*()-style name

jennybc · 2023-04-03T23:52:06Z

  expect_snapshot(cli::cli_bullets(bulletize(letters[1:8], n_fudge = 3)))
 })
+
+# menu(), but based on readline() + cli and mockable ---------------------------


This basically covers the cli_menu() stuff 100%, except the lines you can only hit in interactive use.

gaborcsardi

👍

hadley

Looks like some nice improvements!

hadley · 2023-04-04T19:56:34Z

-  ))
-  choice <- utils::menu(emails)
+    c(
+      "Select a pre-authorised account or enter '0' to obtain a new token.",


Should we add an explicit bullet like "Obtain a new token" instead of using 0? (I don't think this code will work now because cli_menu() will exit to the top-level if the user enters 0)

I don't think this code will work now because cli_menu() will exit to the top-level if the user enters 0

Yes, this is true. I will deal with that 🤔

hadley · 2023-04-04T19:57:44Z

+    if (length(local_input) > 0) {
+      cli::cli_abort(
+        c(x = "Internal error: mocked input is invalid."),
+        .envir = .envir,
+        call = error_call
+      )
+    }


Can we do that all in one place? Or extract it out into a helper?

Sure it's inefficient to re-read the option more than once but this truly does not matter.

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>

hadley · 2023-04-05T02:20:47Z

-      "Select a pre-authorised account or enter '0' to obtain a new token.",
-      "Press Esc/Ctrl + C to cancel."
+      "Enter '1' to start a new auth process or select a pre-authorized account.",
+      "Press Esc or Ctrl + C to cancel."


Maybe you could omit this line now that 0 has its typical behaviour?

jennybc · 2023-04-05T15:19:31Z

+  withr::local_options(
+    rlang_interactive = TRUE,
+    # trailing 0 prevents infinite loop if x only contains invalid choices
+    cli_input = c(x, "0"),


Appending "0" here means you can't get stuck in an infinite loop, which means I can also delete the fussy check inside the repeat { ... } in cli_menu().

Maybe this makes both of us happy? 😅

Or that could go in cli_readline()? I think I forgot how these functions were factored.

Yeah, that's an interesting idea. I've mentally moved on to other projects in gargle, but I think this conversation and the state of these functions here can still inform whatever a more official version looks like.

jennybc · 2023-04-05T15:21:02Z

I found a solution I like to the infinite loop problem, which is to append "0" to any mocked user input. And this retains the ability to provide multiple inputs.

The original intent was to be able to supply multiple values that were used in turn.

I also added a test for this behaviour.

Introduce cli_menu() and friends

99704cc

jennybc commented Apr 3, 2023

View reviewed changes

jennybc requested review from gaborcsardi and hadley April 3, 2023 23:53

jennybc marked this pull request as ready for review April 3, 2023 23:54

gaborcsardi approved these changes Apr 4, 2023

View reviewed changes

hadley approved these changes Apr 4, 2023

View reviewed changes

jennybc and others added 7 commits April 4, 2023 16:25

Keep all of this in one place

a9bb679

Sure it's inefficient to re-read the option more than once but this truly does not matter.

Update R/Gargle-class.R

10654d2

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>

Deal with long line

e2ec764

Merge branch 'main' into cli-menu

485e0e4

Make "go to the browser" an explicit choice

163f92b

Merge branch 'main' into cli-menu

53e3f7f

Merge branch 'main' into cli-menu

d5c8219

hadley reviewed Apr 5, 2023

View reviewed changes

jennybc added 3 commits April 4, 2023 21:17

Assume the user knows how to exit (via '0')

0d747f8

Use correct option name, oops

f5bcd5e

Append '0' to mocked user input

71e9627

jennybc commented Apr 5, 2023

View reviewed changes

jennybc added 2 commits April 5, 2023 08:29

Add a test for multiple valid inputs

6e688e5

Add NEWS bullet

c2fb24b

jennybc merged commit 21c11ee into main Apr 5, 2023

jennybc deleted the cli-menu branch April 5, 2023 15:41

jennybc mentioned this pull request Dec 12, 2023

use rstudioapi instead of readline if calling oauth_flow_auth_code() from RStudio IDE r-lib/httr2#410

Closed

		# guard against invalid mocked input
		local_input <- getOption("cli_input", character())

Uh oh!

Conversation

jennybc commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborcsardi left a comment

Choose a reason for hiding this comment

Uh oh!

hadley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennybc commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jennybc commented Apr 3, 2023 •

edited

Loading

jennybc commented Apr 5, 2023 •

edited

Loading