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

@@ -200,6 +200,7 @@ pub struct FmtFlags {
   pub prose_wrap: Option<String>,
   pub no_semicolons: Option<bool>,
   pub watch: Option<WatchFlags>,
+  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::<bool>("single-quote");
   let prose_wrap = matches.remove_one::<String>("prose-wrap");
   let no_semicolons = matches.remove_one::<bool>("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 =
+    let r = flags_from_vec(svec![
-      flags_from_vec(svec!["deno", "fmt", "--watch", "--no-clear-screen"]);
+      "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()),

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 =
+      let mut another_unstable_flags = Vec::from([
-        Vec::from(["sloppy-imports", "byonm", "bare-node-builtins"]);
+        "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);
 

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 {

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<Option<String>, 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(
+          "yml" | "yaml" => {
+            if unstable_options.yaml {
+              pretty_yaml::format_text(
                 text,
                 &get_resolved_yaml_config(fmt_options),
               )
               .map(Some)
-          .map_err(AnyError::from),
+              .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<Option<String>, 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(
+    "yml" | "yaml" => {
+      if unstable_options.yaml {
+        pretty_yaml::format_text(
           file_text,
           &get_resolved_yaml_config(fmt_options),
         )
         .map(Some)
-    .map_err(AnyError::from),
+        .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<PathBuf>,
     fmt_options: FmtOptionsConfig,
+    unstable_options: UnstableFmtOptions,
     incremental_cache: Arc<IncrementalCache>,
   ) -> Result<(), AnyError>;
 
@@ -358,6 +383,7 @@ impl Formatter for CheckFormatter {
     &self,
     paths: Vec<PathBuf>,
     fmt_options: FmtOptionsConfig,
+    unstable_options: UnstableFmtOptions,
     incremental_cache: Arc<IncrementalCache>,
   ) -> 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<PathBuf>,
     fmt_options: FmtOptionsConfig,
+    unstable_options: UnstableFmtOptions,
     incremental_cache: Arc<IncrementalCache>,
   ) -> 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,
+          |file_path, file_text| {
-          format_file,
+            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) -> Result<Option<String>, AnyError>,
-  fmt_func: impl Fn(
-    &Path,
-    &str,
-    &FmtOptionsConfig,
-  ) -> Result<Option<String>, AnyError>,
 ) -> Result<Option<String>, 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, &current_text, fmt_options) {
+        match fmt_func(file_path, &current_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(
+    format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, file_text| {
-      &PathBuf::from("mod.ts"),
+      Ok(Some(format!("1{file_text}")))
-      "1",
+    })
-      &Default::default(),
-      |_, file_text, _| Ok(Some(format!("1{file_text}"))),
-    )
     .unwrap();
   }
 
   #[test]
   fn test_format_ensure_stable_error_first() {
-    let err = format_ensure_stable(
+    let err = format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, _| {
-      &PathBuf::from("mod.ts"),
+      bail!("Error formatting.")
-      "1",
+    })
-      &Default::default(),
-      |_, _, _| 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(
+    format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, file_text| {
-      &PathBuf::from("mod.ts"),
-      "1",
-      &Default::default(),
-      |_, 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(
+    let result =
-      &PathBuf::from("mod.ts"),
+      format_ensure_stable(&PathBuf::from("mod.ts"), "1", |_, file_text| {
-      "1",
-      &Default::default(),
-      |_, file_text, _| {
         if file_text == "1" {
           Ok(Some("11".to_string()))
         } else if file_text == "11" {
@@ -937,8 +956,7 @@ mod test {
         } else {
           unreachable!();
         }
-      },
+      })
-    )
       .unwrap();
 
     assert_eq!(result, Some("11".to_string()));
@@ -953,6 +971,7 @@ mod test {
         single_quote: Some(true),
         ..Default::default()
       },
+      &UnstableFmtOptions::default(),
     )
     .unwrap()
     .unwrap();

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(),

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();

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"
+      }]
+    }
+  }
+}

tests/specs/fmt/unstable_yaml/badly_formatted.yml

@@ -0,0 +1,3 @@
+- Test
+ - Test
+  - Test