Repository: hirosystems/clarinet (language: rust)

Issue:
Clarinet format with define-trait has unreadable output
**Describe the bug**

I couldn't wait to try out the new format command, and I ran it on my contracts. I noticed a couple of things, the first one being that define-traits are kinda minified.

```clarity
;; title: sigle-post-trait-v1
;; version: 1
;; website: https://sigle.io

(define-trait sigle-post-trait
    (
        (mint (principal) (response uint uint))
        (burn (uint) (response bool uint))

        (set-mint-enabled (bool) (response bool uint))
        (reduce-supply (uint) (response bool uint))
        (get-minter () (response principal uint))
        (get-version () (response uint uint))

        (get-contract-owner () (response principal uint))
        (transfer-ownership ((optional principal)) (response bool uint))
        (accept-ownership () (response bool uint))
    )
)
```

Output

```clarity
;; title: sigle-post-trait-v1
;; version: 1
;; website: https://sigle.io
(define-trait sigle-post-trait
  ((mint (principal) (response uint uint)) (burn (uint) (response bool uint)) (set-mint-enabled (bool) (response bool uint)) (reduce-supply (uint) (response bool uint)) (get-minter () (response principal uint)) (get-version () (response uint uint)) (get-contract-owner () (response principal uint)) (transfer-ownership ((optional principal)) (response bool uint)) (accept-ownership () (response bool uint)))
```

**Expected behavior**

The output should be easy to read and not in one line only.




Discussion / hints:
Thanks for testing and reporting this issue Leo!
We'll soon open a GtiHub discussion to gather feedback, but this is indeed a bug (not just a feedback), thanks for the issue

Can you ping me once the discussion is created? Have some feedback that is not really a bug but more like general preferences :)

Fix patch:
diff --git a/components/clarinet-format/src/formatter/mod.rs b/components/clarinet-format/src/formatter/mod.rs
index c50d7b46d..3cf41e0cd 100644
--- a/components/clarinet-format/src/formatter/mod.rs
+++ b/components/clarinet-format/src/formatter/mod.rs
@@ -279,7 +279,10 @@ impl<'a> Aggregator<'a> {
                             | DefineFunctions::PrivateFunction => self.function(list),
                             DefineFunctions::Constant
                             | DefineFunctions::PersistedVariable
-                            | DefineFunctions::NonFungibleToken => self.constant(list),
+                            | DefineFunctions::FungibleToken
+                            | DefineFunctions::NonFungibleToken => {
+                                self.constant(list, previous_indentation)
+                            }
                             DefineFunctions::Map => self.format_map(list, previous_indentation),
                             DefineFunctions::UseTrait | DefineFunctions::ImplTrait => {
                                 // these are the same as the following but need a trailing newline
@@ -288,9 +291,6 @@ impl<'a> Aggregator<'a> {
                                     self.format_source_exprs(list, previous_indentation)
                                 )
                             }
-                            DefineFunctions::FungibleToken => {
-                                self.fungible_token(list, previous_indentation)
-                            }
                             DefineFunctions::Trait => self.define_trait(list, previous_indentation),
                         }
                     } else {
@@ -319,27 +319,38 @@ impl<'a> Aggregator<'a> {
     fn define_trait(&self, exprs: &[PreSymbolicExpression], previous_indentation: &str) -> String {
         let mut acc = "(define-trait ".to_string();
         let indentation = &self.settings.indentation.to_string();
+        let space = format!("{}{}", indentation, previous_indentation);
         acc.push_str(&self.format_source_exprs(&[exprs[1].clone()], previous_indentation));
-        let mut iter = exprs[2..].iter().peekable();
+        acc.push('\n');
+        acc.push_str(&space);
+        acc.push('(');
+        let mut iter = exprs[2].match_list().unwrap().iter().peekable();
         while let Some(expr) = iter.next() {
-            let trailing = get_trailing_comment(expr, &mut iter);
-            acc.push('\n');
-            acc.push_str(indentation);
-            acc.push_str(&self.format_source_exprs(&[expr.clone()], indentation));
-            if let Some(comment) = trailing {
-                acc.push(' ');
-                acc.push_str(&self.display_pse(comment, previous_indentation));
+            if let Some(list) = expr.match_list() {
+                let trailing = get_trailing_comment(expr, &mut iter);
+                acc.push('\n');
+                acc.push_str(&space);
+                acc.push_str(indentation);
+                acc.push_str(&self.format_list(list, indentation));
+
+                if let Some(comment) = trailing {
+                    acc.push(' ');
+                    acc.push_str(&self.display_pse(comment, previous_indentation));
+                }
             }
         }
+        acc.push('\n');
+        acc.push_str(&space);
+        acc.push(')');
+        acc.push('\n');
+        acc.push_str(previous_indentation);
+        acc.push(')');
         acc
     }
 
-    fn fungible_token(
-        &self,
-        exprs: &[PreSymbolicExpression],
-        previous_indentation: &str,
-    ) -> String {
-        let mut acc = "(define-fungible-token ".to_string();
+    fn constant(&self, exprs: &[PreSymbolicExpression], previous_indentation: &str) -> String {
+        let func_type = self.display_pse(exprs.first().unwrap(), "");
+        let mut acc = format!("({func_type} ");
         let mut iter = exprs[1..].iter().peekable();
         while let Some(expr) = iter.next() {
             let trailing = get_trailing_comment(expr, &mut iter);
@@ -353,39 +364,9 @@ impl<'a> Aggregator<'a> {
             }
         }
         acc.push(')');
+        acc.push('\n');
         acc
     }
-    fn constant(&self, exprs: &[PreSymbolicExpression]) -> String {
-        let func_type = self.display_pse(exprs.first().unwrap(), "");
-        let indentation = &self.settings.indentation.to_string();
-        let mut acc = format!("({func_type} ");
-
-        if let Some((name, args)) = name_and_args(exprs) {
-            acc.push_str(&self.display_pse(name, ""));
-
-            // Access the value from args
-            if let Some(value) = args.first() {
-                if let Some(list) = value.match_list() {
-                    acc.push_str(&format!(
-                        "\n{}({})",
-                        indentation,
-                        self.format_source_exprs(list, "")
-                    ));
-                    acc.push_str("\n)");
-                } else {
-                    // Handle non-list values (e.g., literals or simple expressions)
-                    acc.push(' ');
-                    acc.push_str(&self.display_pse(value, ""));
-                    acc.push(')');
-                }
-            }
-
-            acc.push('\n');
-            acc
-        } else {
-            panic!("Expected a valid constant definition with (name value)")
-        }
-    }
     fn format_map(&self, exprs: &[PreSymbolicExpression], previous_indentation: &str) -> String {
         let mut acc = "(define-map ".to_string();
         let indentation = &self.settings.indentation.to_string();
@@ -707,7 +688,13 @@ impl<'a> Aggregator<'a> {
     fn display_pse(&self, pse: &PreSymbolicExpression, previous_indentation: &str) -> String {
         match pse.pre_expr {
             PreSymbolicExpressionType::Atom(ref value) => t(value.as_str()).to_string(),
-            PreSymbolicExpressionType::AtomValue(ref value) => value.to_string(),
+            PreSymbolicExpressionType::AtomValue(ref value) => match value {
+                clarity::vm::types::Value::Principal(c) => {
+                    format!("'{}", c.to_string())
+                }
+                // Fill in these explicitly
+                _ => value.to_string(),
+            },
             PreSymbolicExpressionType::List(ref items) => {
                 self.format_list(items, previous_indentation)
             }
@@ -936,11 +923,11 @@ mod tests_formatter {
 
     #[test]
     fn test_fungible_token() {
-        let src = "(define-fungible-token hello)";
+        let src = "(define-fungible-token hello)\n";
         let result = format_with_default(&String::from(src));
         assert_eq!(result, src);
 
-        let src = "(define-fungible-token hello u100)";
+        let src = "(define-fungible-token hello u100)\n";
         let result = format_with_default(&String::from(src));
         assert_eq!(result, src);
     }
@@ -1205,12 +1192,9 @@ mod tests_formatter {
     }
     #[test]
     fn test_constant() {
-        let src = "(define-constant something 1)\n";
+        let src = "(define-constant minter 'ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.minter)\n";
         let result = format_with_default(&String::from(src));
-        assert_eq!(result, "(define-constant something 1)\n");
-        let src2 = "(define-constant something (1 2))\n";
-        let result2 = format_with_default(&String::from(src2));
-        assert_eq!(result2, "(define-constant something\n  (1 2)\n)\n");
+        assert_eq!(result, src);
     }
 
     #[test]
@@ -1317,11 +1301,16 @@ mod tests_formatter {
     }
 
     #[test]
-    #[ignore]
+    fn define_data_var_test() {
+        let src = "(define-data-var my-data-var principal tx-sender)\n";
+        let result = format_with_default(src);
+        assert_eq!(src, result);
+    }
+    #[test]
     fn define_trait_test() {
-        // TODO: Not sure how this should be formatted
         let src = r#"(define-trait token-trait
-  ((transfer? (principal principal uint) (response uint uint))
+  (
+    (transfer? (principal principal uint) (response uint uint)) ;; comment
     (get-balance (principal) (response uint uint))
   )
 )"#;


Test patch:
diff --git a/components/clarinet-format/tests/golden-intended/BNS-V2.clar b/components/clarinet-format/tests/golden-intended/BNS-V2.clar
index 4a4c48c44..f5939b006 100644
--- a/components/clarinet-format/tests/golden-intended/BNS-V2.clar
+++ b/components/clarinet-format/tests/golden-intended/BNS-V2.clar
@@ -28,9 +28,7 @@
 );; Only authorized caller to flip the switch and update URI
 (define-constant DEPLOYER tx-sender)
 ;; (new) Var to store the token URI, allowing for metadata association with the NFT
-(define-data-var token-uri
-    (string-ascii 256)
-)
+(define-data-var token-uri (string-ascii 256) "ipfs://QmUQY1aZ799SPRaNBFqeCvvmZ4fTQfZvWHauRvHAukyQDB")
 (define-public (update-token-uri
         (new-token-uri (string-ascii 256))
     )
@@ -41,9 +39,7 @@
         ))
 )
 
-(define-data-var contract-uri
-    (string-ascii 256)
-)
+(define-data-var contract-uri (string-ascii 256) "ipfs://QmWKTZEMQNWngp23i7bgPzkineYC9LDvcxYkwNyVQVoH8y")
 (define-public (update-contract-uri
         (new-contract-uri (string-ascii 256))
     )
@@ -55,104 +51,42 @@
 )
 
 ;; errors
-(define-constant ERR-UNWRAP
-    (err u101)
-)
-(define-constant ERR-NOT-AUTHORIZED
-    (err u102)
-)
-(define-constant ERR-NOT-LISTED
-    (err u103)
-)
-(define-constant ERR-WRONG-COMMISSION
-    (err u104)
-)
-(define-constant ERR-LISTED
-    (err u105)
-)
-(define-constant ERR-NO-NAME
-    (err u106)
-)
-(define-constant ERR-HASH-MALFORMED
-    (err u107)
-)
-(define-constant ERR-STX-BURNT-INSUFFICIENT
-    (err u108)
-)
-(define-constant ERR-PREORDER-NOT-FOUND
-    (err u109)
-)
-(define-constant ERR-CHARSET-INVALID
-    (err u110)
-)
-(define-constant ERR-NAMESPACE-ALREADY-EXISTS
-    (err u111)
-)
-(define-constant ERR-PREORDER-CLAIMABILITY-EXPIRED
-    (err u112)
-)
-(define-constant ERR-NAMESPACE-NOT-FOUND
-    (err u113)
-)
-(define-constant ERR-OPERATION-UNAUTHORIZED
-    (err u114)
-)
-(define-constant ERR-NAMESPACE-ALREADY-LAUNCHED
-    (err u115)
-)
-(define-constant ERR-NAMESPACE-PREORDER-LAUNCHABILITY-EXPIRED
-    (err u116)
-)
-(define-constant ERR-NAMESPACE-NOT-LAUNCHED
-    (err u117)
-)
-(define-constant ERR-NAME-NOT-AVAILABLE
-    (err u118)
-)
-(define-constant ERR-NAMESPACE-BLANK
-    (err u119)
-)
-(define-constant ERR-NAME-BLANK
-    (err u120)
-)
-(define-constant ERR-NAME-PREORDERED-BEFORE-NAMESPACE-LAUNCH
-    (err u121)
-)
-(define-constant ERR-NAMESPACE-HAS-MANAGER
-    (err u122)
-)
-(define-constant ERR-OVERFLOW
-    (err u123)
-)
-(define-constant ERR-NO-NAMESPACE-MANAGER
-    (err u124)
-)
-(define-constant ERR-FAST-MINTED-BEFORE
-    (err u125)
-)
-(define-constant ERR-PREORDERED-BEFORE
-    (err u126)
-)
-(define-constant ERR-NAME-NOT-CLAIMABLE-YET
-    (err u127)
-)
-(define-constant ERR-IMPORTED-BEFORE
-    (err u128)
-)
-(define-constant ERR-LIFETIME-EQUAL-0
-    (err u129)
-)
-(define-constant ERR-MIGRATION-IN-PROGRESS
-    (err u130)
-)
-(define-constant ERR-NO-PRIMARY-NAME
-    (err u131)
-)
+(define-constant ERR-UNWRAP (err u101))
+(define-constant ERR-NOT-AUTHORIZED (err u102))
+(define-constant ERR-NOT-LISTED (err u103))
+(define-constant ERR-WRONG-COMMISSION (err u104))
+(define-constant ERR-LISTED (err u105))
+(define-constant ERR-NO-NAME (err u106))
+(define-constant ERR-HASH-MALFORMED (err u107))
+(define-constant ERR-STX-BURNT-INSUFFICIENT (err u108))
+(define-constant ERR-PREORDER-NOT-FOUND (err u109))
+(define-constant ERR-CHARSET-INVALID (err u110))
+(define-constant ERR-NAMESPACE-ALREADY-EXISTS (err u111))
+(define-constant ERR-PREORDER-CLAIMABILITY-EXPIRED (err u112))
+(define-constant ERR-NAMESPACE-NOT-FOUND (err u113))
+(define-constant ERR-OPERATION-UNAUTHORIZED (err u114))
+(define-constant ERR-NAMESPACE-ALREADY-LAUNCHED (err u115))
+(define-constant ERR-NAMESPACE-PREORDER-LAUNCHABILITY-EXPIRED (err u116))
+(define-constant ERR-NAMESPACE-NOT-LAUNCHED (err u117))
+(define-constant ERR-NAME-NOT-AVAILABLE (err u118))
+(define-constant ERR-NAMESPACE-BLANK (err u119))
+(define-constant ERR-NAME-BLANK (err u120))
+(define-constant ERR-NAME-PREORDERED-BEFORE-NAMESPACE-LAUNCH (err u121))
+(define-constant ERR-NAMESPACE-HAS-MANAGER (err u122))
+(define-constant ERR-OVERFLOW (err u123))
+(define-constant ERR-NO-NAMESPACE-MANAGER (err u124))
+(define-constant ERR-FAST-MINTED-BEFORE (err u125))
+(define-constant ERR-PREORDERED-BEFORE (err u126))
+(define-constant ERR-NAME-NOT-CLAIMABLE-YET (err u127))
+(define-constant ERR-IMPORTED-BEFORE (err u128))
+(define-constant ERR-LIFETIME-EQUAL-0 (err u129))
+(define-constant ERR-MIGRATION-IN-PROGRESS (err u130))
+(define-constant ERR-NO-PRIMARY-NAME (err u131))
 ;; variables
 ;; (new) Variable to see if migration is complete
-(define-data-var migration-complete bool)
+(define-data-var migration-complete bool false)
 ;; (new) Counter to keep track of the last minted NFT ID, ensuring unique identifiers
-(define-data-var bns-index uint)
+(define-data-var bns-index uint u0)
 ;; maps
 ;; (new) Map to track market listings, associating NFT IDs with price and commission details
 (define-map market