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.

Sign up for GitHub

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

Jump to bottom

ui: refactor prompt to write to stdout or stderr #4536

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

ui: refactor prompt to write to stdout or stderr #4536

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e916c6e
ui: simplify stripping logic and get rid of warning
torquestomp Sep 25, 2024
8debeae
ui: control where prompt messages are written
torquestomp Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cli/src/git_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ pub fn is_colocated_git_workspace(workspace: &Workspace, repo: &ReadonlyRepo) ->
}

fn terminal_get_username(ui: &Ui, url: &str) -> Option<String> {
ui.prompt(&format!("Username for {url}")).ok()
ui.prompt(&mut ui.stdout(), &format!("Username for {url}"))
.ok()
}

fn terminal_get_pw(ui: &Ui, url: &str) -> Option<String> {
Expand Down
1 change: 1 addition & 0 deletions cli/src/movement_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ fn choose_commit<'a>(
drop(formatter);

let choice = ui.prompt_choice(
&mut ui.stdout(),
"enter the index of the commit you want to target",
&choices,
None,
Expand Down
19 changes: 13 additions & 6 deletions cli/src/ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,15 +566,15 @@ impl Ui {
.unwrap_or(false)
}

pub fn prompt(&self, prompt: &str) -> io::Result<String> {
pub fn prompt(&self, writer: &mut dyn Write, prompt: &str) -> io::Result<String> {
if !Self::can_prompt() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate the intent? It's weird that can_prompt() tests stdout, but we use an arbitrary write stream.

If the goal was to handle the case where stdout is redirected, it might be okay to always use stderr. Maybe we can also use some library that can obtain tty even if stdout/err was redirected. I assume rpassword does something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume its for testing Google's custom binary. But I agree that its missing the intent in the commit message.

return Err(io::Error::new(
io::ErrorKind::Unsupported,
"Cannot prompt for input since the output is not connected to a terminal",
));
}
write!(self.stdout(), "{prompt}: ")?;
self.stdout().flush()?;
write!(writer, "{prompt}: ")?;
writer.flush()?;
let mut buf = String::new();
io::stdin().read_line(&mut buf)?;

Expand All @@ -592,20 +592,21 @@ impl Ui {
/// Repeat the given prompt until the input is one of the specified choices.
pub fn prompt_choice(
&self,
writer: &mut dyn Write,
prompt: &str,
choices: &[impl AsRef<str>],
default: Option<&str>,
) -> io::Result<String> {
if !Self::can_prompt() {
if let Some(default) = default {
// Choose the default automatically without waiting.
writeln!(self.stdout(), "{prompt}: {default}")?;
writeln!(writer, "{prompt}: {default}")?;
return Ok(default.to_owned());
}
}

loop {
let choice = self.prompt(prompt)?.trim().to_owned();
let choice = self.prompt(writer, prompt)?.trim().to_owned();
if choice.is_empty() {
if let Some(default) = default {
return Ok(default.to_owned());
Expand All @@ -620,7 +621,12 @@ impl Ui {
}

/// Prompts for a yes-or-no response, with yes = true and no = false.
pub fn prompt_yes_no(&self, prompt: &str, default: Option<bool>) -> io::Result<bool> {
pub fn prompt_yes_no(
&self,
writer: &mut dyn Write,
prompt: &str,
default: Option<bool>,
) -> io::Result<bool> {
let default_str = match &default {
Some(true) => "(Yn)",
Some(false) => "(yN)",
Expand All @@ -629,6 +635,7 @@ impl Ui {
let default_choice = default.map(|c| if c { "Y" } else { "N" });

let choice = self.prompt_choice(
writer,
&format!("{} {}", prompt, default_str),
&["y", "n", "yes", "no", "Yes", "No", "YES", "NO"],
default_choice,
Expand Down