From 0da81205d57e947e82aa02206f8ba6822c624ebc Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 2 Aug 2024 15:52:48 +0200 Subject: [PATCH] feat(unstable/fmt): move yaml formatting behind unstable flag (#24848) This moves YAML formatting behind an unstable flag for Deno 1.46. This will make it opt-in to start and then we can remove the flag to make it on by default in version of Deno after that. This can be specified by doing `deno fmt --unstable-yaml` or by specifying the following in a deno.json file: ```json { "unstable": ["fmt-yaml"] } ``` --- cli/args/flags.rs | 29 +++- cli/args/mod.rs | 33 +++- cli/lsp/language_server.rs | 18 ++- cli/tools/fmt.rs | 141 ++++++++++-------- tests/integration/fmt_tests.rs | 3 + tests/integration/upgrade_tests.rs | 2 +- tests/specs/fmt/unstable_yaml/__test__.jsonc | 25 ++++ .../fmt/unstable_yaml/badly_formatted.yml | 3 + 8 files changed, 185 insertions(+), 69 deletions(-) create mode 100644 tests/specs/fmt/unstable_yaml/__test__.jsonc create mode 100644 tests/specs/fmt/unstable_yaml/badly_formatted.yml diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 6affa9f086..e283abc24e 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -200,6 +200,7 @@ pub struct FmtFlags { pub prose_wrap: Option, pub no_semicolons: Option, pub watch: Option, + pub unstable_yaml: bool, } impl FmtFlags { @@ -2074,6 +2075,13 @@ Ignore formatting a file by adding an ignore comment at the top of the file: "Don't use semicolons except where necessary. Defaults to false.", ), ) + .arg( + Arg::new("unstable-yaml") + .long("unstable-yaml") + .help("Enable formatting YAML files.") + .value_parser(FalseyValueParser::new()) + .action(ArgAction::SetTrue), + ) }) } @@ -4088,6 +4096,7 @@ fn fmt_parse(flags: &mut Flags, matches: &mut ArgMatches) { let single_quote = matches.remove_one::("single-quote"); let prose_wrap = matches.remove_one::("prose-wrap"); let no_semicolons = matches.remove_one::("no-semicolons"); + let unstable_yaml = matches.get_flag("unstable-yaml"); flags.subcommand = DenoSubcommand::Fmt(FmtFlags { check: matches.get_flag("check"), @@ -4099,6 +4108,7 @@ fn fmt_parse(flags: &mut Flags, matches: &mut ArgMatches) { prose_wrap, no_semicolons, watch: watch_arg_parse(matches), + unstable_yaml, }); } @@ -5781,6 +5791,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Default::default(), }), ext: Some("ts".to_string()), @@ -5804,6 +5815,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Default::default(), }), ext: Some("ts".to_string()), @@ -5827,6 +5839,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Default::default(), }), ext: Some("ts".to_string()), @@ -5850,6 +5863,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Some(Default::default()), }), ext: Some("ts".to_string()), @@ -5857,8 +5871,13 @@ mod tests { } ); - let r = - flags_from_vec(svec!["deno", "fmt", "--watch", "--no-clear-screen"]); + let r = flags_from_vec(svec![ + "deno", + "fmt", + "--watch", + "--no-clear-screen", + "--unstable-yaml" + ]); assert_eq!( r.unwrap(), Flags { @@ -5874,6 +5893,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: true, watch: Some(WatchFlags { hmr: false, no_clear_screen: true, @@ -5908,6 +5928,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Some(Default::default()), }), ext: Some("ts".to_string()), @@ -5931,6 +5952,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Default::default(), }), ext: Some("ts".to_string()), @@ -5962,6 +5984,7 @@ mod tests { single_quote: None, prose_wrap: None, no_semicolons: None, + unstable_yaml: false, watch: Some(Default::default()), }), config_flag: ConfigFlag::Path("deno.jsonc".to_string()), @@ -5998,6 +6021,7 @@ mod tests { single_quote: Some(true), prose_wrap: Some("never".to_string()), no_semicolons: Some(true), + unstable_yaml: false, watch: Default::default(), }), ext: Some("ts".to_string()), @@ -6028,6 +6052,7 @@ mod tests { single_quote: Some(false), prose_wrap: None, no_semicolons: Some(false), + unstable_yaml: false, watch: Default::default(), }), ext: Some("ts".to_string()), diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 3d35d8c36d..fcc1a8d7c3 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -279,9 +279,15 @@ impl BenchOptions { } } +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] +pub struct UnstableFmtOptions { + pub yaml: bool, +} + #[derive(Clone, Debug)] pub struct FmtOptions { pub options: FmtOptionsConfig, + pub unstable: UnstableFmtOptions, pub files: FilePatterns, } @@ -295,13 +301,21 @@ impl FmtOptions { pub fn new_with_base(base: PathBuf) -> Self { Self { options: FmtOptionsConfig::default(), + unstable: Default::default(), files: FilePatterns::new_with_base(base), } } - pub fn resolve(fmt_config: FmtConfig, fmt_flags: &FmtFlags) -> Self { + pub fn resolve( + fmt_config: FmtConfig, + unstable: UnstableFmtOptions, + fmt_flags: &FmtFlags, + ) -> Self { Self { options: resolve_fmt_options(fmt_flags, fmt_config.options), + unstable: UnstableFmtOptions { + yaml: unstable.yaml || fmt_flags.unstable_yaml, + }, files: fmt_config.files, } } @@ -1306,14 +1320,21 @@ impl CliOptions { let member_configs = self .workspace() .resolve_fmt_config_for_members(&cli_arg_patterns)?; + let unstable = self.resolve_config_unstable_fmt_options(); let mut result = Vec::with_capacity(member_configs.len()); for (ctx, config) in member_configs { - let options = FmtOptions::resolve(config, fmt_flags); + let options = FmtOptions::resolve(config, unstable.clone(), fmt_flags); result.push((ctx, options)); } Ok(result) } + pub fn resolve_config_unstable_fmt_options(&self) -> UnstableFmtOptions { + UnstableFmtOptions { + yaml: self.workspace().has_unstable("fmt-yaml"), + } + } + pub fn resolve_workspace_lint_options( &self, lint_flags: &LintFlags, @@ -1640,8 +1661,12 @@ impl CliOptions { .map(|granular_flag| granular_flag.0) .collect(); - let mut another_unstable_flags = - Vec::from(["sloppy-imports", "byonm", "bare-node-builtins"]); + let mut another_unstable_flags = Vec::from([ + "sloppy-imports", + "byonm", + "bare-node-builtins", + "fmt-yaml", + ]); // add more unstable flags to the same vector holding granular flags all_valid_unstable_flags.append(&mut another_unstable_flags); diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 62cbd58dd1..314c9ec147 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -92,6 +92,7 @@ use crate::args::CaData; use crate::args::CacheSetting; use crate::args::CliOptions; use crate::args::Flags; +use crate::args::UnstableFmtOptions; use crate::factory::CliFactory; use crate::file_fetcher::FileFetcher; use crate::graph_util; @@ -1361,6 +1362,16 @@ impl Inner { .clone(); fmt_options.use_tabs = Some(!params.options.insert_spaces); fmt_options.indent_width = Some(params.options.tab_size as u8); + let maybe_workspace = self + .config + .tree + .data_for_specifier(&specifier) + .map(|d| &d.member_dir.workspace); + let unstable_options = UnstableFmtOptions { + yaml: maybe_workspace + .map(|w| w.has_unstable("fmt-yaml")) + .unwrap_or(false), + }; let document = document.clone(); move || { let format_result = match document.maybe_parsed_source() { @@ -1378,7 +1389,12 @@ impl Inner { .map(|ext| file_path.with_extension(ext)) .unwrap_or(file_path); // it's not a js/ts file, so attempt to format its contents - format_file(&file_path, document.content(), &fmt_options) + format_file( + &file_path, + document.content(), + &fmt_options, + &unstable_options, + ) } }; match format_result { diff --git a/cli/tools/fmt.rs b/cli/tools/fmt.rs index 43a97a72ed..c4eebddf7a 100644 --- a/cli/tools/fmt.rs +++ b/cli/tools/fmt.rs @@ -13,6 +13,7 @@ use crate::args::FmtFlags; use crate::args::FmtOptions; use crate::args::FmtOptionsConfig; use crate::args::ProseWrap; +use crate::args::UnstableFmtOptions; use crate::cache::Caches; use crate::colors; use crate::factory::CliFactory; @@ -58,7 +59,11 @@ pub async fn format( let start_dir = &cli_options.start_dir; let fmt_config = start_dir .to_fmt_config(FilePatterns::new_with_base(start_dir.dir_path()))?; - let fmt_options = FmtOptions::resolve(fmt_config, &fmt_flags); + let fmt_options = FmtOptions::resolve( + fmt_config, + cli_options.resolve_config_unstable_fmt_options(), + &fmt_flags, + ); return format_stdin( &fmt_flags, fmt_options, @@ -184,11 +189,16 @@ async fn format_files( let paths = paths_with_options.paths; let incremental_cache = Arc::new(IncrementalCache::new( caches.fmt_incremental_cache_db(), - &fmt_options.options, + &(&fmt_options.options, &fmt_options.unstable), // cache key &paths, )); formatter - .handle_files(paths, fmt_options.options, incremental_cache.clone()) + .handle_files( + paths, + fmt_options.options, + fmt_options.unstable, + incremental_cache.clone(), + ) .await?; incremental_cache.wait_completion().await; } @@ -212,6 +222,7 @@ fn collect_fmt_files( fn format_markdown( file_text: &str, fmt_options: &FmtOptionsConfig, + unstable_options: &UnstableFmtOptions, ) -> Result, AnyError> { let markdown_config = get_resolved_markdown_config(fmt_options); dprint_plugin_markdown::format_text( @@ -252,12 +263,18 @@ fn format_markdown( json_config.line_width = line_width; dprint_plugin_json::format_text(&fake_filename, text, &json_config) } - "yml" | "yaml" => pretty_yaml::format_text( - text, - &get_resolved_yaml_config(fmt_options), - ) - .map(Some) - .map_err(AnyError::from), + "yml" | "yaml" => { + if unstable_options.yaml { + pretty_yaml::format_text( + text, + &get_resolved_yaml_config(fmt_options), + ) + .map(Some) + .map_err(AnyError::from) + } else { + Ok(None) + } + } _ => { let mut codeblock_config = get_resolved_typescript_config(fmt_options); @@ -293,24 +310,31 @@ pub fn format_file( file_path: &Path, file_text: &str, fmt_options: &FmtOptionsConfig, + unstable_options: &UnstableFmtOptions, ) -> Result, AnyError> { let ext = get_extension(file_path).unwrap_or_default(); match ext.as_str() { "md" | "mkd" | "mkdn" | "mdwn" | "mdown" | "markdown" => { - format_markdown(file_text, fmt_options) + format_markdown(file_text, fmt_options, unstable_options) } "json" | "jsonc" => format_json(file_path, file_text, fmt_options), - "yml" | "yaml" => pretty_yaml::format_text( - file_text, - &get_resolved_yaml_config(fmt_options), - ) - .map(Some) - .map_err(AnyError::from), + "yml" | "yaml" => { + if unstable_options.yaml { + pretty_yaml::format_text( + file_text, + &get_resolved_yaml_config(fmt_options), + ) + .map(Some) + .map_err(AnyError::from) + } else { + Ok(None) + } + } "ipynb" => dprint_plugin_jupyter::format_text( file_text, |file_path: &Path, file_text: String| { - format_file(file_path, &file_text, fmt_options) + format_file(file_path, &file_text, fmt_options, unstable_options) }, ), _ => { @@ -340,6 +364,7 @@ trait Formatter { &self, paths: Vec, fmt_options: FmtOptionsConfig, + unstable_options: UnstableFmtOptions, incremental_cache: Arc, ) -> Result<(), AnyError>; @@ -358,6 +383,7 @@ impl Formatter for CheckFormatter { &self, paths: Vec, fmt_options: FmtOptionsConfig, + unstable_options: UnstableFmtOptions, incremental_cache: Arc, ) -> Result<(), AnyError> { // prevent threads outputting at the same time @@ -375,7 +401,12 @@ impl Formatter for CheckFormatter { return Ok(()); } - match format_file(&file_path, &file_text, &fmt_options) { + match format_file( + &file_path, + &file_text, + &fmt_options, + &unstable_options, + ) { Ok(Some(formatted_text)) => { not_formatted_files_count.fetch_add(1, Ordering::Relaxed); let _g = output_lock.lock(); @@ -450,6 +481,7 @@ impl Formatter for RealFormatter { &self, paths: Vec, fmt_options: FmtOptionsConfig, + unstable_options: UnstableFmtOptions, incremental_cache: Arc, ) -> Result<(), AnyError> { let output_lock = Arc::new(Mutex::new(0)); // prevent threads outputting at the same time @@ -469,8 +501,9 @@ impl Formatter for RealFormatter { match format_ensure_stable( &file_path, &file_contents.text, - &fmt_options, - format_file, + |file_path, file_text| { + format_file(file_path, file_text, &fmt_options, &unstable_options) + }, ) { Ok(Some(formatted_text)) => { incremental_cache.update_file(&file_path, &formatted_text); @@ -527,20 +560,15 @@ impl Formatter for RealFormatter { fn format_ensure_stable( file_path: &Path, file_text: &str, - fmt_options: &FmtOptionsConfig, - fmt_func: impl Fn( - &Path, - &str, - &FmtOptionsConfig, - ) -> Result, AnyError>, + fmt_func: impl Fn(&Path, &str) -> Result, AnyError>, ) -> Result, AnyError> { - let formatted_text = fmt_func(file_path, file_text, fmt_options)?; + let formatted_text = fmt_func(file_path, file_text)?; match formatted_text { Some(mut current_text) => { let mut count = 0; loop { - match fmt_func(file_path, ¤t_text, fmt_options) { + match fmt_func(file_path, ¤t_text) { Ok(Some(next_pass_text)) => { // just in case if next_pass_text == current_text { @@ -595,7 +623,12 @@ fn format_stdin( bail!("Failed to read from stdin"); } let file_path = PathBuf::from(format!("_stdin.{ext}")); - let formatted_text = format_file(&file_path, &source, &fmt_options.options)?; + let formatted_text = format_file( + &file_path, + &source, + &fmt_options.options, + &fmt_options.unstable, + )?; if fmt_flags.check { #[allow(clippy::print_stdout)] if formatted_text.is_some() { @@ -883,23 +916,17 @@ mod test { #[test] #[should_panic(expected = "Formatting not stable. Bailed after 5 tries.")] fn test_format_ensure_stable_unstable_format() { - format_ensure_stable( - &PathBuf::from("mod.ts"), - "1", - &Default::default(), - |_, file_text, _| Ok(Some(format!("1{file_text}"))), - ) + format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, file_text| { + Ok(Some(format!("1{file_text}"))) + }) .unwrap(); } #[test] fn test_format_ensure_stable_error_first() { - let err = format_ensure_stable( - &PathBuf::from("mod.ts"), - "1", - &Default::default(), - |_, _, _| bail!("Error formatting."), - ) + let err = format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, _| { + bail!("Error formatting.") + }) .unwrap_err(); assert_eq!(err.to_string(), "Error formatting."); @@ -908,28 +935,20 @@ mod test { #[test] #[should_panic(expected = "Formatting succeeded initially, but failed when")] fn test_format_ensure_stable_error_second() { - format_ensure_stable( - &PathBuf::from("mod.ts"), - "1", - &Default::default(), - |_, file_text, _| { - if file_text == "1" { - Ok(Some("11".to_string())) - } else { - bail!("Error formatting.") - } - }, - ) + format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, file_text| { + if file_text == "1" { + Ok(Some("11".to_string())) + } else { + bail!("Error formatting.") + } + }) .unwrap(); } #[test] fn test_format_stable_after_two() { - let result = format_ensure_stable( - &PathBuf::from("mod.ts"), - "1", - &Default::default(), - |_, file_text, _| { + let result = + format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, file_text| { if file_text == "1" { Ok(Some("11".to_string())) } else if file_text == "11" { @@ -937,9 +956,8 @@ mod test { } else { unreachable!(); } - }, - ) - .unwrap(); + }) + .unwrap(); assert_eq!(result, Some("11".to_string())); } @@ -953,6 +971,7 @@ mod test { single_quote: Some(true), ..Default::default() }, + &UnstableFmtOptions::default(), ) .unwrap() .unwrap(); diff --git a/tests/integration/fmt_tests.rs b/tests/integration/fmt_tests.rs index 17adef6f84..dab2b2ce45 100644 --- a/tests/integration/fmt_tests.rs +++ b/tests/integration/fmt_tests.rs @@ -49,6 +49,7 @@ fn fmt_test() { .current_dir(&testdata_fmt_dir) .args_vec(vec![ "fmt".to_string(), + "--unstable-yaml".to_string(), format!( "--ignore={badly_formatted_js},{badly_formatted_md},{badly_formatted_json},{badly_formatted_yaml},{badly_formatted_ipynb}", ), @@ -69,6 +70,7 @@ fn fmt_test() { .args_vec(vec![ "fmt".to_string(), "--check".to_string(), + "--unstable-yaml".to_string(), badly_formatted_js.to_string(), badly_formatted_md.to_string(), badly_formatted_json.to_string(), @@ -86,6 +88,7 @@ fn fmt_test() { .current_dir(&testdata_fmt_dir) .args_vec(vec![ "fmt".to_string(), + "--unstable-yaml".to_string(), badly_formatted_js.to_string(), badly_formatted_md.to_string(), badly_formatted_json.to_string(), diff --git a/tests/integration/upgrade_tests.rs b/tests/integration/upgrade_tests.rs index 844401d4e2..6af73f65fc 100644 --- a/tests/integration/upgrade_tests.rs +++ b/tests/integration/upgrade_tests.rs @@ -145,7 +145,7 @@ fn upgrade_with_out_in_tmpdir() { assert!(v.contains("1.11.5")); } -#[test] +#[flaky_test::flaky_test] fn upgrade_invalid_stable_version() { let context = upgrade_context(); let temp_dir = context.temp_dir(); diff --git a/tests/specs/fmt/unstable_yaml/__test__.jsonc b/tests/specs/fmt/unstable_yaml/__test__.jsonc new file mode 100644 index 0000000000..885db59b98 --- /dev/null +++ b/tests/specs/fmt/unstable_yaml/__test__.jsonc @@ -0,0 +1,25 @@ +{ + "tempDir": true, + "tests": { + "nothing": { + "args": "fmt", + "output": "Checked 1 file\n" + }, + "flag": { + "args": "fmt --unstable-yaml", + "output": "[WILDLINE]badly_formatted.yml\nChecked 1 file\n" + }, + "config_file": { + "steps": [{ + "args": [ + "eval", + "Deno.writeTextFile('deno.json', '{\\n \"unstable\": [\"fmt-yaml\"]\\n}\\n')" + ], + "output": "[WILDCARD]" + }, { + "args": "fmt", + "output": "[WILDLINE]badly_formatted.yml\nChecked 2 files\n" + }] + } + } +} \ No newline at end of file diff --git a/tests/specs/fmt/unstable_yaml/badly_formatted.yml b/tests/specs/fmt/unstable_yaml/badly_formatted.yml new file mode 100644 index 0000000000..49646f320f --- /dev/null +++ b/tests/specs/fmt/unstable_yaml/badly_formatted.yml @@ -0,0 +1,3 @@ +- Test + - Test + - Test \ No newline at end of file