Repository: toml-rs/toml (language: rust)

Issue:
`easy::to_string_pretty` doesn't produce pretty results



Discussion / hints:
I propose that `Document`, `Table,`, etc have `fmt` functions that take a format configuration.  `to_string_pretty` and other [serializer options](https://docs.rs/toml/0.5.8/toml/ser/struct.Serializer.html) are implemented in terms of this.

This function will be the public facing one.  An internal one will be used when recursing so we can pass in the relevant information from the caller, in addition to the format config.
We should port the following tests when we do this
- https://github.com/alexcrichton/toml-rs/blob/master/test-suite/tests/formatting.rs
- https://github.com/alexcrichton/toml-rs/blob/master/test-suite/tests/pretty.rs
Hello, I've just touched this. Let me state my understanding of the question and see if my understanding is correct.

It seems that you don't think the printout of the current structure, such as ```Table``` and ```document```, is perfect. Therefore, the function ```easy::to_string_pretty``` needs to be optimized, right?

If my understanding is correct, what is the perfect output format you want?
Please give me a detailed description. Thank you.
I'd say we start with mimicing `toml-rs`, ie able to pass their tests
- https://github.com/alexcrichton/toml-rs/blob/master/test-suite/tests/formatting.rs
- https://github.com/alexcrichton/toml-rs/blob/master/test-suite/tests/pretty.rs

We can look to their serde implementation for a starting point for the algorithm: https://github.com/alexcrichton/toml-rs/blob/master/src/ser.rs

Without looking to closely
- We should always create valid toml
- We should prefer array of tables over inline arrays
- If a table has too many elements, we should prefer standard tables over inline tables.
Example failure from cargo's test suite
```
---- features_namespaced::publish stdout ----                                                                                                      
running `/home/epage/src/personal/cargo/target/debug/cargo publish --token sekrit -Z namespaced-features`                                          
thread 'features_namespaced::publish' panicked at '                      
                                                                                                                                                   
error:  did not match:                                                                                                                             
1   1     # THIS FILE IS AUTOMATICALLY GENERATED BY CARGO                                                                                          
2   2     #                                                                                                                                        
3   3     # When uploading crates to the registry Cargo will automatically                                                                         
4   4     # "normalize" Cargo.toml files for maximal compatibility                                                                                 
5   5     # with all versions of Cargo and also rewrite `path` dependencies                                                                        
6   6     # to registry (e.g., crates.io) dependencies.                                                                                            
7   7     #                                                                                                                                        
8   8     # If you are reading this file be aware that the original Cargo.toml                                                                     
9   9     # will likely look very different (and much more reasonable).                                                                            
10  10    # See Cargo.toml.orig for the original contents.                                                                                         
11  11                                                                                                                                             
12       -[package]                                                                                                                                
13       -name = "foo"                                                                                                                             
14       -version = "0.1.0"                                                                                                                        
15       -description = "foo"                                                                                                                      
16       -homepage = "https://example.com/"                              
17       -license = "MIT"                                                                                                                          
18       -[dependencies.bar]                                                                                                                       
19       -version = "1.0"                                                                                                                          
20       -optional = true                                                                                                                          
21       -                                                                                                                                         
22       -[features]                                                                                                                               
23       -feat1 = []                                                                                                                               
24       -feat2 = ["dep:bar"]                                                                                                                      
25       -feat3 = ["feat2"]                                                                                                                        
    12   +package = { name = "foo", version = "0.1.0", description = "foo", homepage = "https://example.com/", license = "MIT"}                    
    13   +dependencies = { bar = { version = "1.0", optional = true}}                                                                              
    14   +features = { feat1 = [], feat2 = ["dep:bar"], feat3 = ["feat2"]}                                                                         
```
Thanks! I ran across this while trying to see if I can switch my code to using `toml_edit` (which looks fantastic btw!)

I'm wondering if at some point, finer-grained control might be desirable -- perhaps some sort of trait that can control whether a particular table is serialized as inline or standard. e.g. for a `Cargo.toml`, I'd like to see `dependencies.bar` be an inline table but `features` be a standard table.
>  I'm wondering if at some point, finer-grained control might be desirable -- perhaps some sort of trait that can control whether a particular table is serialized as inline or standard. e.g. for a Cargo.toml, I'd like to see dependencies.bar be an inline table but features be a standard table.

That makes sense.  A similar thought i had was a way to control sort order (e.g. `[package]`, `[features]`, `[dependencies]`, `[target]`).

The main thing is balancing the complexity, whether thats in a user finding what they want and understanding how to use it, implementing the users code on top of the API, or the implementation within the library.   At a certain point of proliferation of customization, it seems like it'd be better for someone to walk the data structure and format it manually. I guess another aspect of all of this is finding the answer to what can we do to support people writing that custom formatting.

btw guppy looks interesting! In particular, I find the idea of determinator interesting from two angles
- For build/test avoidance in open source.  For example, clap's CI takes over an hour (they did switch to a two-tier CI using bors so that cost is only paid on merge and not each PR update, instead its only a half hour or so which is still bad). I would love to also have coverage-driven test case avoidance but thats its own separate thing.
- As a maintainer of one of the release automation tools (cargo-release).  We've all been implementing our own bespoke implementations of release-avoidance.  These have their own needs.  For a release, all you care about is included/excluded files.  You also only need to release all packages with a `[[bin]]` if the lock file changed.  I wish there was a way to know if a dependency showed up in a public API.
> That makes sense. A similar thought i had was a way to control sort order (e.g. [package], [features], [dependencies], [target]).

> The main thing is balancing the complexity, whether thats in a user finding what they want and understanding how to use it, implementing the users code on top of the API, or the implementation within the library. At a certain point of proliferation of customization, it seems like it'd be better for someone to walk the data structure and format it manually. I guess another aspect of all of this is finding the answer to what can we do to support people writing that custom formatting.

Oh that's a super interesting idea. I wonder what an API based on [`syn::visit_mut`](https://docs.rs/syn/1.0.81/syn/visit_mut/index.html) would feel like. Going to try playing with it a bit over the next few days.

> btw guppy looks interesting! In particular, I find the idea of determinator interesting from two angles

Thank you! Yeah, the determinator is really cool and has proven to work quite well in a large workspace. I think both of the use cases you described make a lot of sense.

I haven't seen much demand for it in open source yet but now that you've asked for it, I now have a reason to clean it up and put it out there :) Happy to chat about this in more detail on Discord or some other medium.

Fix patch:
diff --git a/src/item.rs b/src/item.rs
index e3017aa5..ac0601f1 100644
--- a/src/item.rs
+++ b/src/item.rs
@@ -28,6 +28,7 @@ impl Item {
         self
     }
 }
+
 // TODO: This should be generated by macro or derive
 /// Downcasting
 impl Item {
@@ -128,19 +129,15 @@ impl Item {
     }
     /// In-place convert to a value
     pub fn make_value(&mut self) {
-        let mut other = Item::None;
-        std::mem::swap(self, &mut other);
-        let mut other = other.into_value().map(Item::Value).unwrap_or(Item::None);
-        std::mem::swap(self, &mut other);
+        let other = std::mem::take(self);
+        let other = other.into_value().map(Item::Value).unwrap_or(Item::None);
+        *self = other;
     }
     /// Casts `self` to table.
     pub fn into_table(self) -> Result<Table, Self> {
         match self {
             Item::Table(t) => Ok(t),
-            Item::Value(v) => match v {
-                Value::InlineTable(t) => Ok(t.into_table()),
-                _ => Err(Item::Value(v)),
-            },
+            Item::Value(Value::InlineTable(t)) => Ok(t.into_table()),
             _ => Err(self),
         }
     }
@@ -148,9 +145,36 @@ impl Item {
     pub fn into_array_of_tables(self) -> Result<ArrayOfTables, Self> {
         match self {
             Item::ArrayOfTables(a) => Ok(a),
+            Item::Value(Value::Array(a)) => {
+                if a.is_empty() {
+                    Err(Item::Value(Value::Array(a)))
+                } else if a.iter().all(|v| v.is_inline_table()) {
+                    let mut aot = ArrayOfTables::new();
+                    aot.values = a.values;
+                    for value in aot.values.iter_mut() {
+                        value.make_item();
+                    }
+                    Ok(aot)
+                } else {
+                    Err(Item::Value(Value::Array(a)))
+                }
+            }
             _ => Err(self),
         }
     }
+    // Starting private because the name is unclear
+    pub(crate) fn make_item(&mut self) {
+        let other = std::mem::take(self);
+        let other = match other.into_table().map(crate::Item::Table) {
+            Ok(i) => i,
+            Err(i) => i,
+        };
+        let other = match other.into_array_of_tables().map(crate::Item::ArrayOfTables) {
+            Ok(i) => i,
+            Err(i) => i,
+        };
+        *self = other;
+    }
     /// Returns true iff `self` is a value.
     pub fn is_value(&self) -> bool {
         self.as_value().is_some()
diff --git a/src/ser/mod.rs b/src/ser/mod.rs
index 4f266541..86ea5625 100644
--- a/src/ser/mod.rs
+++ b/src/ser/mod.rs
@@ -5,6 +5,7 @@
 mod array;
 mod item;
 mod key;
+mod pretty;
 mod table;
 
 pub(crate) use array::*;
@@ -12,6 +13,8 @@ pub(crate) use item::*;
 pub(crate) use key::*;
 pub(crate) use table::*;
 
+use crate::visit_mut::VisitMut;
+
 /// Errors that can occur when deserializing a type.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Error {
@@ -146,7 +149,9 @@ pub fn to_string_pretty<T: ?Sized>(value: &T) -> Result<String, Error>
 where
     T: serde::ser::Serialize,
 {
-    to_document(value).map(|e| e.to_string())
+    let mut document = to_document(value)?;
+    pretty::Pretty.visit_document_mut(&mut document);
+    Ok(document.to_string())
 }
 
 /// Serialize the given data structure into a TOML document.
diff --git a/src/ser/pretty.rs b/src/ser/pretty.rs
new file mode 100644
index 00000000..fa4da4e3
--- /dev/null
+++ b/src/ser/pretty.rs
@@ -0,0 +1,56 @@
+pub(crate) struct Pretty;
+
+impl crate::visit_mut::VisitMut for Pretty {
+    fn visit_document_mut(&mut self, node: &mut crate::Document) {
+        crate::visit_mut::visit_document_mut(self, node);
+        if let Some((_, first)) = node.iter_mut().next() {
+            remove_table_prefix(first);
+        }
+    }
+
+    fn visit_item_mut(&mut self, node: &mut crate::Item) {
+        node.make_item();
+
+        crate::visit_mut::visit_item_mut(self, node);
+    }
+
+    fn visit_table_mut(&mut self, node: &mut crate::Table) {
+        node.decor_mut().clear();
+
+        crate::visit_mut::visit_table_mut(self, node);
+    }
+
+    fn visit_value_mut(&mut self, node: &mut crate::Value) {
+        node.decor_mut().clear();
+
+        crate::visit_mut::visit_value_mut(self, node);
+    }
+
+    fn visit_array_mut(&mut self, node: &mut crate::Array) {
+        crate::visit_mut::visit_array_mut(self, node);
+
+        if (0..=1).contains(&node.len()) {
+            node.set_trailing("");
+            node.set_trailing_comma(false);
+        } else {
+            for item in node.iter_mut() {
+                item.decor_mut().set_prefix("\n    ");
+            }
+            node.set_trailing("\n");
+            node.set_trailing_comma(true);
+        }
+    }
+}
+
+fn remove_table_prefix(node: &mut crate::Item) {
+    match node {
+        crate::Item::None => {}
+        crate::Item::Value(_) => {}
+        crate::Item::Table(t) => t.decor_mut().set_prefix(""),
+        crate::Item::ArrayOfTables(a) => {
+            if let Some(first) = a.values.iter_mut().next() {
+                remove_table_prefix(first);
+            }
+        }
+    }
+}


Test patch:
diff --git a/tests/pretty.rs b/tests/pretty.rs
new file mode 100644
index 00000000..77922f90
--- /dev/null
+++ b/tests/pretty.rs
@@ -0,0 +1,110 @@
+#![cfg(feature = "easy")]
+
+use pretty_assertions::assert_eq;
+
+const PRETTY_STD: &str = r#"[example]
+array = [
+    "item 1",
+    "item 2",
+]
+empty = []
+one = ["one"]
+oneline = "this has no newlines."
+text = """
+this is the first line
+this is the second line
+"""
+"#;
+
+#[test]
+fn pretty_std() {
+    let toml = PRETTY_STD;
+    let value: toml_edit::easy::Value = toml_edit::easy::from_str(toml).unwrap();
+    let result = toml_edit::easy::to_string_pretty(&value).unwrap();
+    println!("EXPECTED:\n{}", toml);
+    println!("\nRESULT:\n{}", result);
+    assert_eq!(toml, &result);
+}
+
+const PRETTY_TRICKY: &str = r##"[example]
+f = "\f"
+glass = """
+Nothing too unusual, except that I can eat glass in:
+- Greek: Μπορώ να φάω σπασμένα γυαλιά χωρίς να πάθω τίποτα. 
+- Polish: Mogę jeść szkło, i mi nie szkodzi. 
+- Hindi: मैं काँच खा सकता हूँ, मुझे उस से कोई पीडा नहीं होती. 
+- Japanese: 私はガラスを食べられます。それは私を傷つけません。 
+"""
+r = "\r"
+r_newline = """
+\r
+"""
+single = "this is a single line but has \"\" cuz it\"s tricky"
+single_tricky = "single line with ''' in it"
+tabs = """
+this is pretty standard
+\texcept for some \ttabs right here
+"""
+text = """
+this is the first line.
+This has a ''' in it and \"\"\" cuz it's tricky yo
+Also ' and \" because why not
+this is the fourth line
+"""
+"##;
+
+#[test]
+fn pretty_tricky() {
+    let toml = PRETTY_TRICKY;
+    let value: toml_edit::easy::Value = toml_edit::easy::from_str(toml).unwrap();
+    let result = toml_edit::easy::to_string_pretty(&value).unwrap();
+    println!("EXPECTED:\n{}", toml);
+    println!("\nRESULT:\n{}", result);
+    assert_eq!(toml, &result);
+}
+
+const PRETTY_TABLE_ARRAY: &str = r##"[[array]]
+key = "foo"
+
+[[array]]
+key = "bar"
+
+[abc]
+doc = "this is a table"
+
+[example]
+single = "this is a single line string"
+"##;
+
+#[test]
+fn pretty_table_array() {
+    let toml = PRETTY_TABLE_ARRAY;
+    let value: toml_edit::easy::Value = toml_edit::easy::from_str(toml).unwrap();
+    let result = toml_edit::easy::to_string_pretty(&value).unwrap();
+    println!("EXPECTED:\n{}", toml);
+    println!("\nRESULT:\n{}", result);
+    assert_eq!(toml, &result);
+}
+
+const TABLE_ARRAY: &str = r##"[[array]]
+key = "foo"
+
+[[array]]
+key = "bar"
+
+[abc]
+doc = "this is a table"
+
+[example]
+single = "this is a single line string"
+"##;
+
+#[test]
+fn table_array() {
+    let toml = TABLE_ARRAY;
+    let value: toml_edit::easy::Value = toml_edit::easy::from_str(toml).unwrap();
+    let result = toml_edit::easy::to_string_pretty(&value).unwrap();
+    println!("EXPECTED:\n{}", toml);
+    println!("\nRESULT:\n{}", result);
+    assert_eq!(toml, &result);
+}
diff --git a/tests/serde.rs b/tests/serde.rs
index d9557792..4d1d0d4b 100644
--- a/tests/serde.rs
+++ b/tests/serde.rs
@@ -29,9 +29,15 @@ macro_rules! equivalent {
 
         // Through a string equivalent
         println!("to_string(literal)");
-        assert_eq!(t!(toml_edit::easy::to_string(&literal)), toml.to_string());
+        assert_eq!(
+            t!(toml_edit::easy::to_string_pretty(&literal)),
+            toml.to_string()
+        );
         println!("to_string(toml)");
-        assert_eq!(t!(toml_edit::easy::to_string(&toml)), toml.to_string());
+        assert_eq!(
+            t!(toml_edit::easy::to_string_pretty(&toml)),
+            toml.to_string()
+        );
         println!("literal, from_str(toml)");
         assert_eq!(literal, t!(toml_edit::easy::from_str(&toml.to_string())));
         println!("toml, from_str(toml)");