From 3d8a63e0723287d68de9f9192fd683645de23465 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Mon, 28 Apr 2025 18:07:39 -0400 Subject: [PATCH] minimize diff A few edits here to minimize the overall (squashed) diff: * Minimize borrow-checker-satisfying changes in write_impl, write_all_impl. We just need to "refresh" the bindings of `doc` and `view` It's nice to avoid extra blocks for the `&mut` bindings since they unnecessarily add indentation/nesting. * Factor out the `Result, _>` early in `Document::format`: this avoids changing the indentation for the rest of the block. With these changes the stat (git diff --stat 9f3b193743e6) lowers from +128,-82 to +51,-10. Also there's a style change here to prefer `use`-ing the `crate::expansion` module so it's clearer where the `expand` function comes from. --- helix-term/src/commands/typed.rs | 56 +++++++------- helix-view/src/document.rs | 123 +++++++++++++++---------------- 2 files changed, 87 insertions(+), 92 deletions(-) diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index 0652daf62..c35ff714a 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -324,22 +324,20 @@ fn buffer_previous( fn write_impl(cx: &mut compositor::Context, path: Option<&str>, force: bool) -> anyhow::Result<()> { let config = cx.editor.config(); let jobs = &mut cx.jobs; - { - let (view, doc) = current!(cx.editor); + let (view, doc) = current!(cx.editor); - if doc.trim_trailing_whitespace() { - trim_trailing_whitespace(doc, view.id); - } - if config.trim_final_newlines { - trim_final_newlines(doc, view.id); - } - if doc.insert_final_newline() { - insert_final_newline(doc, view.id); - } - - // Save an undo checkpoint for any outstanding changes. - doc.append_changes_to_history(view); + if doc.trim_trailing_whitespace() { + trim_trailing_whitespace(doc, view.id); } + if config.trim_final_newlines { + trim_final_newlines(doc, view.id); + } + if doc.insert_final_newline() { + insert_final_newline(doc, view.id); + } + + // Save an undo checkpoint for any outstanding changes. + doc.append_changes_to_history(view); let (view, doc) = current_ref!(cx.editor); let fmt = if config.auto_format { @@ -738,26 +736,24 @@ pub fn write_all_impl( .collect(); for (doc_id, target_view) in saves { - { - let doc = doc_mut!(cx.editor, &doc_id); - let view = view_mut!(cx.editor, target_view); + let doc = doc_mut!(cx.editor, &doc_id); + let view = view_mut!(cx.editor, target_view); - if doc.trim_trailing_whitespace() { - trim_trailing_whitespace(doc, target_view); - } - if config.trim_final_newlines { - trim_final_newlines(doc, target_view); - } - if doc.insert_final_newline() { - insert_final_newline(doc, target_view); - } - - // Save an undo checkpoint for any outstanding changes. - doc.append_changes_to_history(view); + if doc.trim_trailing_whitespace() { + trim_trailing_whitespace(doc, target_view); + } + if config.trim_final_newlines { + trim_final_newlines(doc, target_view); + } + if doc.insert_final_newline() { + insert_final_newline(doc, target_view); } - let doc = doc!(cx.editor, &doc_id); + // Save an undo checkpoint for any outstanding changes. + doc.append_changes_to_history(view); + let fmt = if options.auto_format && config.auto_format { + let doc = doc!(cx.editor, &doc_id); doc.auto_format(cx.editor).map(|fmt| { let callback = make_format_callback( doc_id, diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 61b064f13..ae31bab60 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -5,7 +5,6 @@ use futures_util::future::BoxFuture; use futures_util::FutureExt; use helix_core::auto_pairs::AutoPairs; use helix_core::chars::char_is_word; - use helix_core::command_line::Token; use helix_core::diagnostic::DiagnosticProvider; use helix_core::doc_formatter::TextFormat; @@ -44,10 +43,10 @@ use helix_core::{ ChangeSet, Diagnostic, LineEnding, Range, Rope, RopeBuilder, Selection, Syntax, Transaction, }; -use crate::expansion::expand; use crate::{ editor::Config, events::{DocumentDidChange, SelectionDidChange}, + expansion, view::ViewPosition, DocumentId, Editor, Theme, View, ViewId, }; @@ -823,68 +822,68 @@ impl Document { process.current_dir(doc_dir); } - let fmt_args = fmt_args + let args = match fmt_args .iter() - .map(|content| expand(editor, Token::expand(content))) - .collect::>>(); - - match fmt_args { - Ok(fmt_args) => { - process - .args(fmt_args.iter().map(Cow::as_ref)) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()); - - let formatting_future = async move { - let mut process = - process - .spawn() - .map_err(|e| FormatterError::SpawningFailed { - command: fmt_cmd.to_string_lossy().into(), - error: e.kind(), - })?; - - let mut stdin = process.stdin.take().ok_or(FormatterError::BrokenStdin)?; - let input_text = text.clone(); - let input_task = tokio::spawn(async move { - to_writer(&mut stdin, (encoding::UTF_8, false), &input_text).await - // Note that `stdin` is dropped here, causing the pipe to close. This can - // avoid a deadlock with `wait_with_output` below if the process is waiting on - // stdin to close before exiting. - }); - let (input_result, output_result) = tokio::join! { - input_task, - process.wait_with_output(), - }; - let _ = input_result.map_err(|_| FormatterError::BrokenStdin)?; - let output = - output_result.map_err(|_| FormatterError::WaitForOutputFailed)?; - - if !output.status.success() { - if !output.stderr.is_empty() { - let err = String::from_utf8_lossy(&output.stderr).to_string(); - log::error!("Formatter error: {}", err); - return Err(FormatterError::NonZeroExitStatus(Some(err))); - } - - return Err(FormatterError::NonZeroExitStatus(None)); - } else if !output.stderr.is_empty() { - log::debug!( - "Formatter printed to stderr: {}", - String::from_utf8_lossy(&output.stderr) - ); - } - - let str = std::str::from_utf8(&output.stdout) - .map_err(|_| FormatterError::InvalidUtf8Output)?; - - Ok(helix_core::diff::compare_ropes(&text, &Rope::from(str))) - }; - return Some(formatting_future.boxed()); + .map(|content| expansion::expand(editor, Token::expand(content))) + .collect::, _>>() + { + Ok(args) => args, + Err(err) => { + log::error!("Failed to expand formatter arguments: {err}"); + return None; } - Err(e) => log::error!("{e}"), - } + }; + + process + .args(args.iter().map(AsRef::as_ref)) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let formatting_future = async move { + let mut process = process + .spawn() + .map_err(|e| FormatterError::SpawningFailed { + command: fmt_cmd.to_string_lossy().into(), + error: e.kind(), + })?; + + let mut stdin = process.stdin.take().ok_or(FormatterError::BrokenStdin)?; + let input_text = text.clone(); + let input_task = tokio::spawn(async move { + to_writer(&mut stdin, (encoding::UTF_8, false), &input_text).await + // Note that `stdin` is dropped here, causing the pipe to close. This can + // avoid a deadlock with `wait_with_output` below if the process is waiting on + // stdin to close before exiting. + }); + let (input_result, output_result) = tokio::join! { + input_task, + process.wait_with_output(), + }; + let _ = input_result.map_err(|_| FormatterError::BrokenStdin)?; + let output = output_result.map_err(|_| FormatterError::WaitForOutputFailed)?; + + if !output.status.success() { + if !output.stderr.is_empty() { + let err = String::from_utf8_lossy(&output.stderr).to_string(); + log::error!("Formatter error: {}", err); + return Err(FormatterError::NonZeroExitStatus(Some(err))); + } + + return Err(FormatterError::NonZeroExitStatus(None)); + } else if !output.stderr.is_empty() { + log::debug!( + "Formatter printed to stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + let str = std::str::from_utf8(&output.stdout) + .map_err(|_| FormatterError::InvalidUtf8Output)?; + + Ok(helix_core::diff::compare_ropes(&text, &Rope::from(str))) + }; + return Some(formatting_future.boxed()); }; let text = self.text.clone();