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

ui: remove Option<_> wrapping from ui.hint_() helpers #3900

Merged
merged 5 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 17 additions & 23 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1937,15 +1937,13 @@ pub fn print_checkout_stats(
working copy.",
stats.skipped_files
)?;
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
"Inspect the changes compared to the intended target with `jj diff --from {}`.
writeln!(
ui.hint_default(),
"Inspect the changes compared to the intended target with `jj diff --from {}`.
Discard the conflicting changes with `jj restore --from {}`.",
short_commit_hash(new_commit.id()),
short_commit_hash(new_commit.id())
)?;
}
short_commit_hash(new_commit.id()),
short_commit_hash(new_commit.id())
)?;
}
Ok(())
}
Expand Down Expand Up @@ -1993,21 +1991,17 @@ pub fn print_trackable_remote_branches(ui: &Ui, view: &View) -> io::Result<()> {
return Ok(());
}

if let Some(mut writer) = ui.hint_default() {
if let Some(mut formatter) = ui.status_formatter() {
writeln!(
writer,
formatter.labeled("hint").with_heading("Hint: "),
"The following remote branches aren't associated with the existing local branches:"
)?;
}
if let Some(mut formatter) = ui.status_formatter() {
for full_name in &remote_branch_names {
write!(formatter, " ")?;
writeln!(formatter.labeled("branch"), "{full_name}")?;
}
}
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
formatter.labeled("hint").with_heading("Hint: "),
"Run `jj branch track {names}` to keep local branches updated on future pulls.",
names = remote_branch_names.join(" "),
)?;
Expand Down Expand Up @@ -2487,14 +2481,14 @@ fn resolve_default_command(
if matches.subcommand_name().is_none() {
let args = get_string_or_array(config, "ui.default-command").optional()?;
if args.is_none() {
if let Some(mut writer) = ui.hint_default() {
writeln!(writer, "Use `jj -h` for a list of available commands.")?;
writeln!(
writer,
"Run `jj config set --user ui.default-command log` to disable this \
message."
)?;
}
writeln!(
ui.hint_default(),
"Use `jj -h` for a list of available commands."
)?;
writeln!(
ui.hint_no_heading(),
"Run `jj config set --user ui.default-command log` to disable this message."
)?;
}
let default_command = args.unwrap_or_else(|| vec!["log".to_string()]);

Expand Down
38 changes: 16 additions & 22 deletions cli/src/commands/branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,14 +329,12 @@ fn cmd_branch_rename(
ui.warning_default(),
"Branch {old_branch} has tracking remote branches which were not renamed."
)?;
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
"to rename the branch on the remote, you can `jj git push --branch {old_branch}` \
first (to delete it on the remote), and then `jj git push --branch \
{new_branch}`. `jj git push --all` would also be sufficient."
)?;
}
writeln!(
ui.hint_default(),
"to rename the branch on the remote, you can `jj git push --branch {old_branch}` \
first (to delete it on the remote), and then `jj git push --branch {new_branch}`. \
`jj git push --all` would also be sufficient."
)?;
}

Ok(())
Expand Down Expand Up @@ -770,21 +768,17 @@ fn cmd_branch_list(
// Print only one of these hints. It's not important to mention unexported
// branches, but user might wonder why deleted branches are still listed.
if found_deleted_tracking_local_branch {
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
"Branches marked as deleted will be *deleted permanently* on the remote on the \
next `jj git push`. Use `jj branch forget` to prevent this."
)?;
}
writeln!(
ui.hint_default(),
"Branches marked as deleted will be *deleted permanently* on the remote on the next \
`jj git push`. Use `jj branch forget` to prevent this."
)?;
} else if found_deleted_local_branch {
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
"Branches marked as deleted will be deleted from the underlying Git repo on the \
next `jj git export`."
)?;
}
writeln!(
ui.hint_default(),
"Branches marked as deleted will be deleted from the underlying Git repo on the next \
`jj git export`."
)?;
}

Ok(())
Expand Down
18 changes: 9 additions & 9 deletions cli/src/commands/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -608,9 +608,10 @@ fn get_default_fetch_remotes(
} else if let Some(remote) = get_single_remote(git_repo)? {
// if nothing was explicitly configured, try to guess
if remote != DEFAULT_REMOTE {
if let Some(mut writer) = ui.hint_default() {
writeln!(writer, "Fetching from the only existing remote: {remote}")?;
}
writeln!(
ui.hint_default(),
"Fetching from the only existing remote: {remote}"
)?;
}
Ok(vec![remote])
} else {
Expand Down Expand Up @@ -1073,9 +1074,10 @@ fn get_default_push_remote(
} else if let Some(remote) = get_single_remote(git_repo)? {
// similar to get_default_fetch_remotes
if remote != DEFAULT_REMOTE {
if let Some(mut writer) = ui.hint_default() {
writeln!(writer, "Pushing to the only existing remote: {remote}")?;
}
writeln!(
ui.hint_default(),
"Pushing to the only existing remote: {remote}"
)?;
}
Ok(remote)
} else {
Expand All @@ -1093,9 +1095,7 @@ impl RejectedBranchUpdateReason {
fn print(&self, ui: &Ui) -> io::Result<()> {
writeln!(ui.warning_default(), "{}", self.message)?;
if let Some(hint) = &self.hint {
if let Some(mut writer) = ui.hint_default() {
writeln!(writer, "{hint}")?;
}
writeln!(ui.hint_default(), "{hint}")?;
}
Ok(())
}
Expand Down
7 changes: 4 additions & 3 deletions cli/src/commands/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,10 @@ fn cmd_util_completion(
"`jj util completion --{shell}` will be removed in a future version, and this will be \
a hard error"
)?;
if let Some(mut writer) = ui.hint_default() {
writeln!(writer, "Use `jj util completion {shell}` instead")?;
}
writeln!(
ui.hint_default(),
"Use `jj util completion {shell}` instead"
)?;
Ok(())
};
let shell = match (args.shell, args.fish, args.zsh, args.bash) {
Expand Down
24 changes: 19 additions & 5 deletions cli/src/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ impl<T, S> LabeledWriter<T, S> {
pub fn new(formatter: T, label: S) -> Self {
LabeledWriter { formatter, label }
}

/// Turns into writer that prints labeled message with the `heading`.
pub fn with_heading<H>(self, heading: H) -> HeadingLabeledWriter<T, S, H> {
HeadingLabeledWriter::new(self, heading)
}
}

impl<'a, T, S> LabeledWriter<T, S>
Expand Down Expand Up @@ -96,9 +101,9 @@ pub struct HeadingLabeledWriter<T, S, H> {
}

impl<T, S, H> HeadingLabeledWriter<T, S, H> {
pub fn new(formatter: T, label: S, heading: H) -> Self {
pub fn new(writer: LabeledWriter<T, S>, heading: H) -> Self {
HeadingLabeledWriter {
writer: LabeledWriter::new(formatter, label),
writer,
heading: Some(heading),
}
}
Expand Down Expand Up @@ -1162,8 +1167,14 @@ mod tests {
let mut output: Vec<u8> = vec![];
let mut formatter: Box<dyn Formatter> =
Box::new(ColorFormatter::for_config(&mut output, &config, false).unwrap());
HeadingLabeledWriter::new(formatter.as_mut(), "inner", "Should be noop: ");
let mut writer = HeadingLabeledWriter::new(formatter.as_mut(), "inner", "Heading: ");
formatter
.as_mut()
.labeled("inner")
.with_heading("Should be noop: ");
let mut writer = formatter
.as_mut()
.labeled("inner")
.with_heading("Heading: ");
write!(writer, "Message").unwrap();
writeln!(writer, " continues").unwrap();
drop(formatter);
Expand All @@ -1176,7 +1187,10 @@ mod tests {
fn test_heading_labeled_writer_empty_string() {
let mut output: Vec<u8> = vec![];
let mut formatter: Box<dyn Formatter> = Box::new(PlainTextFormatter::new(&mut output));
let mut writer = HeadingLabeledWriter::new(formatter.as_mut(), "inner", "Heading: ");
let mut writer = formatter
.as_mut()
.labeled("inner")
.with_heading("Heading: ");
// write_fmt() is called even if the format string is empty. I don't
// know if that's guaranteed, but let's record the current behavior.
write!(writer, "").unwrap();
Expand Down
10 changes: 4 additions & 6 deletions cli/src/git_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,14 +409,12 @@ pub fn print_failed_git_export(
.iter()
.any(|failed| matches!(failed.reason, FailedRefExportReason::FailedToSet(_)))
{
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
r#"Git doesn't allow a branch name that looks like a parent directory of
writeln!(
ui.hint_default(),
r#"Git doesn't allow a branch name that looks like a parent directory of
another (e.g. `foo` and `foo/bar`). Try to rename the branches that failed to
export or their "parent" branches."#,
)?;
}
)?;
}
}
Ok(())
Expand Down
14 changes: 6 additions & 8 deletions cli/src/merge_tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,12 @@ fn editor_args_from_settings(
Ok(args)
} else {
let default_editor = BUILTIN_EDITOR_NAME;
if let Some(mut writer) = ui.hint_default() {
writeln!(
writer,
"Using default editor '{default_editor}'; run `jj config set --user {key} \
:builtin` to disable this message."
)
.ok();
}
writeln!(
ui.hint_default(),
"Using default editor '{default_editor}'; run `jj config set --user {key} :builtin` \
to disable this message."
)
.ok();
Ok(default_editor.into())
}
}
Expand Down
29 changes: 17 additions & 12 deletions cli/src/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ use tracing::instrument;

use crate::command_error::{config_error_with_message, CommandError};
use crate::config::CommandNameAndArgs;
use crate::formatter::{Formatter, FormatterFactory, HeadingLabeledWriter, LabeledWriter};
use crate::formatter::{
Formatter, FormatterFactory, HeadingLabeledWriter, LabeledWriter, PlainTextFormatter,
};

const BUILTIN_PAGER_NAME: &str = ":builtin";

Expand All @@ -47,15 +49,15 @@ pub struct BuiltinPager {
dynamic_pager_thread: JoinHandle<()>,
}

impl std::io::Write for &BuiltinPager {
impl Write for &BuiltinPager {
fn flush(&mut self) -> io::Result<()> {
// no-op since this is being run in a dynamic pager mode.
Ok(())
}

fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
let string = std::str::from_utf8(buf).map_err(std::io::Error::other)?;
self.pager.push_str(string).map_err(std::io::Error::other)?;
let string = std::str::from_utf8(buf).map_err(io::Error::other)?;
self.pager.push_str(string).map_err(io::Error::other)?;
Ok(buf.len())
}
}
Expand Down Expand Up @@ -395,7 +397,7 @@ impl Ui {
/// Writer to print an update that's not part of the command's main output.
pub fn status(&self) -> Box<dyn Write + '_> {
if self.quiet {
Box::new(std::io::sink())
Box::new(io::sink())
} else {
Box::new(self.stderr())
}
Expand All @@ -410,21 +412,24 @@ impl Ui {
/// Writer to print hint with the default "Hint: " heading.
pub fn hint_default(
&self,
) -> Option<HeadingLabeledWriter<Box<dyn Formatter + '_>, &'static str, &'static str>> {
) -> HeadingLabeledWriter<Box<dyn Formatter + '_>, &'static str, &'static str> {
self.hint_with_heading("Hint: ")
}

/// Writer to print hint without the "Hint: " heading.
pub fn hint_no_heading(&self) -> Option<LabeledWriter<Box<dyn Formatter + '_>, &'static str>> {
(!self.quiet).then(|| LabeledWriter::new(self.stderr_formatter(), "hint"))
pub fn hint_no_heading(&self) -> LabeledWriter<Box<dyn Formatter + '_>, &'static str> {
let formatter = self
.status_formatter()
.unwrap_or_else(|| Box::new(PlainTextFormatter::new(io::sink())));
LabeledWriter::new(formatter, "hint")
}

/// Writer to print hint with the given heading.
pub fn hint_with_heading<H: fmt::Display>(
&self,
heading: H,
) -> Option<HeadingLabeledWriter<Box<dyn Formatter + '_>, &'static str, H>> {
(!self.quiet).then(|| HeadingLabeledWriter::new(self.stderr_formatter(), "hint", heading))
) -> HeadingLabeledWriter<Box<dyn Formatter + '_>, &'static str, H> {
self.hint_no_heading().with_heading(heading)
}

/// Writer to print warning with the default "Warning: " heading.
Expand All @@ -444,7 +449,7 @@ impl Ui {
&self,
heading: H,
) -> HeadingLabeledWriter<Box<dyn Formatter + '_>, &'static str, H> {
HeadingLabeledWriter::new(self.stderr_formatter(), "warning", heading)
self.warning_no_heading().with_heading(heading)
}

/// Writer to print error without the "Error: " heading.
Expand All @@ -457,7 +462,7 @@ impl Ui {
&self,
heading: H,
) -> HeadingLabeledWriter<Box<dyn Formatter + '_>, &'static str, H> {
HeadingLabeledWriter::new(self.stderr_formatter(), "error", heading)
self.error_no_heading().with_heading(heading)
}

/// Waits for the pager exits.
Expand Down