From 3f07885b351748c5b8225aadb165f8ef7066f047 Mon Sep 17 00:00:00 2001 From: Skyler Hawthorne Date: Fri, 16 Sep 2022 23:17:48 -0400 Subject: [PATCH] document should save even if formatter fails --- helix-core/src/syntax.rs | 6 ++ helix-term/src/application.rs | 15 ++--- helix-term/src/commands.rs | 27 +++++---- helix-term/src/main.rs | 12 +++- helix-term/tests/test/auto_indent.rs | 1 + helix-term/tests/test/auto_pairs.rs | 1 + helix-term/tests/test/commands.rs | 14 +++-- helix-term/tests/test/helpers.rs | 90 ++++++++++++++++++++++++---- helix-term/tests/test/movement.rs | 4 +- helix-term/tests/test/prompt.rs | 4 +- helix-term/tests/test/splits.rs | 11 +++- helix-term/tests/test/write.rs | 66 +++++++++++++------- 12 files changed, 185 insertions(+), 66 deletions(-) diff --git a/helix-core/src/syntax.rs b/helix-core/src/syntax.rs index 61d382fde..f907629fe 100644 --- a/helix-core/src/syntax.rs +++ b/helix-core/src/syntax.rs @@ -61,6 +61,12 @@ pub struct Configuration { pub language: Vec, } +impl Default for Configuration { + fn default() -> Self { + crate::config::default_syntax_loader() + } +} + // largely based on tree-sitter/cli/src/loader.rs #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "kebab-case", deny_unknown_fields)] diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index fe53d73d7..4fde2a66d 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -1,7 +1,6 @@ use arc_swap::{access::Map, ArcSwap}; use futures_util::Stream; use helix_core::{ - config::{default_syntax_loader, user_syntax_loader}, diagnostic::{DiagnosticTag, NumberOrString}, path::get_relative_path, pos_at_coords, syntax, Selection, @@ -110,7 +109,11 @@ fn restore_term() -> Result<(), Error> { } impl Application { - pub fn new(args: Args, config: Config) -> Result { + pub fn new( + args: Args, + config: Config, + syn_loader_conf: syntax::Configuration, + ) -> Result { #[cfg(feature = "integration")] setup_integration_logging(); @@ -137,14 +140,6 @@ impl Application { }) .unwrap_or_else(|| theme_loader.default_theme(true_color)); - let syn_loader_conf = user_syntax_loader().unwrap_or_else(|err| { - eprintln!("Bad language config: {}", err); - eprintln!("Press to continue with default language config"); - use std::io::Read; - // This waits for an enter press. - let _ = std::io::stdin().read(&mut []); - default_syntax_loader() - }); let syn_loader = std::sync::Arc::new(syntax::Loader::new(syn_loader_conf)); let mut compositor = Compositor::new().context("build compositor")?; diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 6deecbe26..f6d583f51 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -2518,7 +2518,8 @@ async fn make_format_callback( format: impl Future> + Send + 'static, write: Option<(Option, bool)>, ) -> anyhow::Result { - let format = format.await?; + let format = format.await; + let call: job::Callback = Callback::Editor(Box::new(move |editor| { if !editor.documents.contains_key(&doc_id) { return; @@ -2528,19 +2529,21 @@ async fn make_format_callback( let doc = doc_mut!(editor, &doc_id); let view = view_mut!(editor); - if doc.version() == doc_version { - apply_transaction(&format, doc, view); - doc.append_changes_to_history(view.id); - doc.detect_indent_and_line_ending(); - view.ensure_cursor_in_view(doc, scrolloff); + if let Ok(format) = format { + if doc.version() == doc_version { + apply_transaction(&format, doc, view); + doc.append_changes_to_history(view.id); + doc.detect_indent_and_line_ending(); + view.ensure_cursor_in_view(doc, scrolloff); + } else { + log::info!("discarded formatting changes because the document changed"); + } + } - if let Some((path, force)) = write { - if let Err(err) = doc.save(path, force) { - editor.set_error(format!("Error saving: {}", err)); - } + if let Some((path, force)) = write { + if let Err(err) = doc.save(path, force) { + editor.set_error(format!("Error saving: {}", err)); } - } else { - log::info!("discarded formatting changes because the document changed"); } })); diff --git a/helix-term/src/main.rs b/helix-term/src/main.rs index 726bf9e3b..96b695c6f 100644 --- a/helix-term/src/main.rs +++ b/helix-term/src/main.rs @@ -139,8 +139,18 @@ FLAGS: Err(err) => return Err(Error::new(err)), }; + let syn_loader_conf = helix_core::config::user_syntax_loader().unwrap_or_else(|err| { + eprintln!("Bad language config: {}", err); + eprintln!("Press to continue with default language config"); + use std::io::Read; + // This waits for an enter press. + let _ = std::io::stdin().read(&mut []); + helix_core::config::default_syntax_loader() + }); + // TODO: use the thread local executor to spawn the application task separately from the work pool - let mut app = Application::new(args, config).context("unable to create new application")?; + let mut app = Application::new(args, config, syn_loader_conf) + .context("unable to create new application")?; let exit_code = app.run(&mut EventStream::new()).await?; diff --git a/helix-term/tests/test/auto_indent.rs b/helix-term/tests/test/auto_indent.rs index 2f638893c..5c093a5db 100644 --- a/helix-term/tests/test/auto_indent.rs +++ b/helix-term/tests/test/auto_indent.rs @@ -8,6 +8,7 @@ async fn auto_indent_c() -> anyhow::Result<()> { ..Default::default() }, Config::default(), + helpers::test_syntax_conf(None), // switches to append mode? ( helpers::platform_line("void foo() {#[|}]#").as_ref(), diff --git a/helix-term/tests/test/auto_pairs.rs b/helix-term/tests/test/auto_pairs.rs index ec47a5b4a..caf80bd45 100644 --- a/helix-term/tests/test/auto_pairs.rs +++ b/helix-term/tests/test/auto_pairs.rs @@ -13,6 +13,7 @@ async fn auto_pairs_basic() -> anyhow::Result<()> { }, ..Default::default() }, + helpers::test_syntax_conf(None), ("#[\n|]#", "i(", "(#[|\n]#"), ) .await?; diff --git a/helix-term/tests/test/commands.rs b/helix-term/tests/test/commands.rs index 0279e348c..5238cc69b 100644 --- a/helix-term/tests/test/commands.rs +++ b/helix-term/tests/test/commands.rs @@ -1,16 +1,18 @@ use std::ops::RangeInclusive; use helix_core::diagnostic::Severity; -use helix_term::application::Application; use super::*; #[tokio::test(flavor = "multi_thread")] async fn test_write_quit_fail() -> anyhow::Result<()> { let file = helpers::new_readonly_tempfile()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; test_key_sequence( - &mut helpers::app_with_file(file.path())?, + &mut app, Some("ihello:wq"), Some(&|app| { let mut docs: Vec<_> = app.editor.documents().collect(); @@ -30,7 +32,7 @@ async fn test_write_quit_fail() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_buffer_close_concurrent() -> anyhow::Result<()> { test_key_sequences( - &mut Application::new(Args::default(), Config::default())?, + &mut helpers::AppBuilder::new().build()?, vec![ ( None, @@ -70,8 +72,12 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> { command.push_str(":bufferclose"); + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; + test_key_sequence( - &mut helpers::app_with_file(file.path())?, + &mut app, Some(&command), Some(&|app| { assert!(!app.editor.is_err(), "error: {:?}", app.editor.get_status()); diff --git a/helix-term/tests/test/helpers.rs b/helix-term/tests/test/helpers.rs index ed1a03317..c2fbe9536 100644 --- a/helix-term/tests/test/helpers.rs +++ b/helix-term/tests/test/helpers.rs @@ -108,7 +108,7 @@ pub async fn test_key_sequence_with_input_text>( let test_case = test_case.into(); let mut app = match app { Some(app) => app, - None => Application::new(Args::default(), Config::default())?, + None => Application::new(Args::default(), Config::default(), test_syntax_conf(None))?, }; let (view, doc) = helix_view::current!(app.editor); @@ -132,16 +132,30 @@ pub async fn test_key_sequence_with_input_text>( .await } +/// Generates language configs that merge in overrides, like a user language +/// config. The argument string must be a raw TOML document. +pub fn test_syntax_conf(overrides: Option) -> helix_core::syntax::Configuration { + let mut lang = helix_loader::config::default_lang_config(); + + if let Some(overrides) = overrides { + let override_toml = toml::from_str(&overrides).unwrap(); + lang = helix_loader::merge_toml_values(lang, override_toml, 3); + } + + lang.try_into().unwrap() +} + /// Use this for very simple test cases where there is one input /// document, selection, and sequence of key presses, and you just /// want to verify the resulting document and selection. pub async fn test_with_config>( args: Args, config: Config, + syn_conf: helix_core::syntax::Configuration, test_case: T, ) -> anyhow::Result<()> { let test_case = test_case.into(); - let app = Application::new(args, config)?; + let app = Application::new(args, config, syn_conf)?; test_key_sequence_with_input_text( Some(app), @@ -162,7 +176,13 @@ pub async fn test_with_config>( } pub async fn test>(test_case: T) -> anyhow::Result<()> { - test_with_config(Args::default(), Config::default(), test_case).await + test_with_config( + Args::default(), + Config::default(), + test_syntax_conf(None), + test_case, + ) + .await } pub fn temp_file_with_contents>( @@ -207,16 +227,60 @@ pub fn new_readonly_tempfile() -> anyhow::Result { Ok(file) } -/// Creates a new Application with default config that opens the given file -/// path -pub fn app_with_file>(path: P) -> anyhow::Result { - Application::new( - Args { - files: vec![(path.into(), helix_core::Position::default())], - ..Default::default() - }, - Config::default(), - ) +#[derive(Default)] +pub struct AppBuilder { + args: Args, + config: Config, + syn_conf: helix_core::syntax::Configuration, + input: Option<(String, Selection)>, +} + +impl AppBuilder { + pub fn new() -> Self { + AppBuilder::default() + } + + pub fn with_file>( + mut self, + path: P, + pos: Option, + ) -> Self { + self.args.files.push((path.into(), pos.unwrap_or_default())); + self + } + + pub fn with_config(mut self, config: Config) -> Self { + self.config = config; + self + } + + pub fn with_input_text>(mut self, input_text: S) -> Self { + self.input = Some(test::print(&input_text.into())); + self + } + + pub fn with_lang_config(mut self, syn_conf: helix_core::syntax::Configuration) -> Self { + self.syn_conf = syn_conf; + self + } + + pub fn build(self) -> anyhow::Result { + let mut app = Application::new(self.args, self.config, self.syn_conf)?; + + if let Some((text, selection)) = self.input { + let (view, doc) = helix_view::current!(app.editor); + let sel = doc.selection(view.id).clone(); + let trans = Transaction::change_by_selection(doc.text(), &sel, |_| { + (0, doc.text().len_chars(), Some((text.clone()).into())) + }) + .with_selection(selection); + + // replace the initial text with the input text + doc.apply(&trans, view.id); + } + + Ok(app) + } } pub fn assert_file_has_content(file: &mut File, content: &str) -> anyhow::Result<()> { diff --git a/helix-term/tests/test/movement.rs b/helix-term/tests/test/movement.rs index 45aae39e1..7212d026f 100644 --- a/helix-term/tests/test/movement.rs +++ b/helix-term/tests/test/movement.rs @@ -70,7 +70,9 @@ async fn insert_to_normal_mode_cursor_position() -> anyhow::Result<()> { async fn cursor_position_newly_opened_file() -> anyhow::Result<()> { let test = |content: &str, expected_sel: Selection| -> anyhow::Result<()> { let file = helpers::temp_file_with_contents(content)?; - let mut app = helpers::app_with_file(file.path())?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; let (view, doc) = helix_view::current!(app.editor); let sel = doc.selection(view.id).clone(); diff --git a/helix-term/tests/test/prompt.rs b/helix-term/tests/test/prompt.rs index 2ab9604c6..62ec03f1b 100644 --- a/helix-term/tests/test/prompt.rs +++ b/helix-term/tests/test/prompt.rs @@ -1,11 +1,9 @@ use super::*; -use helix_term::application::Application; - #[tokio::test] async fn test_history_completion() -> anyhow::Result<()> { test_key_sequence( - &mut Application::new(Args::default(), Config::default())?, + &mut AppBuilder::new().build()?, Some(":asdf:theme d"), Some(&|app| { assert!(!app.editor.is_err()); diff --git a/helix-term/tests/test/splits.rs b/helix-term/tests/test/splits.rs index 70a517be4..5807413a2 100644 --- a/helix-term/tests/test/splits.rs +++ b/helix-term/tests/test/splits.rs @@ -6,8 +6,12 @@ async fn test_split_write_quit_all() -> anyhow::Result<()> { let mut file2 = tempfile::NamedTempFile::new()?; let mut file3 = tempfile::NamedTempFile::new()?; + let mut app = helpers::AppBuilder::new() + .with_file(file1.path(), None) + .build()?; + test_key_sequences( - &mut helpers::app_with_file(file1.path())?, + &mut app, vec![ ( Some(&format!( @@ -66,9 +70,12 @@ async fn test_split_write_quit_all() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_split_write_quit_same_file() -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; test_key_sequences( - &mut helpers::app_with_file(file.path())?, + &mut app, vec![ ( Some("Oihello:spogoodbye"), diff --git a/helix-term/tests/test/write.rs b/helix-term/tests/test/write.rs index 7d1054316..6aa51a315 100644 --- a/helix-term/tests/test/write.rs +++ b/helix-term/tests/test/write.rs @@ -4,7 +4,6 @@ use std::{ }; use helix_core::diagnostic::Severity; -use helix_term::application::Application; use helix_view::doc; use super::*; @@ -12,9 +11,12 @@ use super::*; #[tokio::test] async fn test_write() -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; test_key_sequence( - &mut helpers::app_with_file(file.path())?, + &mut app, Some("ithe gostak distims the doshes:w"), None, false, @@ -38,9 +40,12 @@ async fn test_write() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_quit() -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; test_key_sequence( - &mut helpers::app_with_file(file.path())?, + &mut app, Some("ithe gostak distims the doshes:wq"), None, true, @@ -66,19 +71,16 @@ async fn test_write_concurrent() -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; let mut command = String::new(); const RANGE: RangeInclusive = 1..=5000; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; for i in RANGE { let cmd = format!("%c{}:w", i); command.push_str(&cmd); } - test_key_sequence( - &mut helpers::app_with_file(file.path())?, - Some(&command), - None, - false, - ) - .await?; + test_key_sequence(&mut app, Some(&command), None, false).await?; file.as_file_mut().flush()?; file.as_file_mut().sync_all()?; @@ -93,9 +95,12 @@ async fn test_write_concurrent() -> anyhow::Result<()> { #[tokio::test] async fn test_write_fail_mod_flag() -> anyhow::Result<()> { let file = helpers::new_readonly_tempfile()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; test_key_sequences( - &mut helpers::app_with_file(file.path())?, + &mut app, vec![ ( None, @@ -133,7 +138,7 @@ async fn test_write_scratch_to_new_path() -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; test_key_sequence( - &mut Application::new(Args::default(), Config::default())?, + &mut AppBuilder::new().build()?, Some(format!("ihello:w {}", file.path().to_string_lossy()).as_ref()), Some(&|app| { assert!(!app.editor.is_err()); @@ -174,19 +179,40 @@ async fn test_write_scratch_no_path_fails() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> { + let mut file = tempfile::Builder::new().suffix(".rs").tempfile()?; + + let lang_conf = indoc! {r#" + [[language]] + name = "rust" + formatter = { command = "bash", args = [ "-c", "exit 1" ] } + "#}; + + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .with_input_text("#[l|]#et foo = 0;\n") + .with_lang_config(helpers::test_syntax_conf(Some(lang_conf.into()))) + .build()?; + + test_key_sequences(&mut app, vec![(Some(":w"), None)], false).await?; + + // file still saves + helpers::assert_file_has_content(file.as_file_mut(), "let foo = 0;\n")?; + + Ok(()) +} + #[tokio::test] async fn test_write_new_path() -> anyhow::Result<()> { let mut file1 = tempfile::NamedTempFile::new().unwrap(); let mut file2 = tempfile::NamedTempFile::new().unwrap(); + let mut app = helpers::AppBuilder::new() + .with_file(file1.path(), None) + .build()?; test_key_sequences( - &mut Application::new( - Args { - files: vec![(file1.path().to_path_buf(), Position::default())], - ..Default::default() - }, - Config::default(), - )?, + &mut app, vec![ ( Some("ii can eat glass, it will not hurt me:w"), @@ -228,7 +254,7 @@ async fn test_write_fail_new_path() -> anyhow::Result<()> { let file = helpers::new_readonly_tempfile()?; test_key_sequences( - &mut Application::new(Args::default(), Config::default())?, + &mut AppBuilder::new().build()?, vec![ ( None,