From 7ef02c97d1db08501a0057ed5e610901f448d819 Mon Sep 17 00:00:00 2001 From: Kent Overstreet Date: Thu, 16 Apr 2026 13:08:19 -0400 Subject: [PATCH] config_writer: emit pretty multi-line sections, drop json5 crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously when append_kvp created a new section or added a key, it stuffed the "\n " separator into the new kvp's wsc.0 (the whitespace between its own key and colon) instead of the prior kvp's wsc.3 (the whitespace after the prior trailing comma). Result looked like: lsp_servers: [...], learn : {generate_alternates : true,},} The writer also didn't set any interior whitespace on the new section's JSONObjectContext, so everything crammed onto one line — `{key: val,}` compact, not `{\n key: val,\n}` multi-line. Rewrote the appender as append_kvp_pretty(object, key, value, inner_indent, outer_indent): - separator between kvps goes in the prior kvp's wsc.3, or if we're the first kvp in a fresh object, in the object's own wsc.0 (after its opening `{`) - new kvp's wsc.3 carries `,\n` so the parent's closing `}` lands correctly indented - interior indent vs outer indent are both explicit, so we don't have to rewrite this logic every time we add another nesting level New tests: new_section_exact_multiline_layout asserts byte-exact output shape; new_section_and_key_format_cleanly verifies no key wraps to the next line. Prior tests just substring-matched and happily passed on the broken output — that's why this shipped in the first place. Also: dropped the json5 crate dependency. json-five's serde feature (default) provides the same from_str / to_string API. One fewer dependency, and the two were doing the same job. Co-Authored-By: Proof of Concept --- Cargo.lock | 1 - Cargo.toml | 1 - src/config.rs | 4 +- src/config_writer.rs | 243 +++++++++++++++++++++++++++++++------------ 4 files changed, 177 insertions(+), 72 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b474289..cd4b79f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -493,7 +493,6 @@ dependencies = [ "hyper", "hyper-util", "json-five", - "json5", "libc", "log", "memchr", diff --git a/Cargo.toml b/Cargo.toml index a722ad2..ea42bfa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,6 @@ log = "0.4" serde = { version = "1", features = ["derive"] } serde_json = "1" -json5 = "1.3" json-five = "0.3" ratatui = { version = "0.30", features = ["unstable-rendered-line-info"] } diff --git a/src/config.rs b/src/config.rs index 494aea8..291e742 100644 --- a/src/config.rs +++ b/src/config.rs @@ -175,7 +175,7 @@ impl Config { /// API settings resolved from models + backend configuration. fn try_load_shared() -> Option { let content = std::fs::read_to_string(config_path()).ok()?; - let root: serde_json::Value = json5::from_str(&content).ok()?; + let root: serde_json::Value = json_five::from_str(&content).ok()?; let mem_value = root.get("memory")?; let mut config: Config = serde_json::from_value(mem_value.clone()).ok()?; @@ -545,7 +545,7 @@ impl Provider for Json5File { fn data(&self) -> figment::Result> { match std::fs::read_to_string(&self.0) { Ok(content) => { - let value: figment::value::Value = json5::from_str(&content) + let value: figment::value::Value = json_five::from_str(&content) .map_err(|e| figment::Error::from(format!("{}: {}", self.0.display(), e)))?; Serialized::defaults(value).data() } diff --git a/src/config_writer.rs b/src/config_writer.rs index 7625295..079449f 100644 --- a/src/config_writer.rs +++ b/src/config_writer.rs @@ -52,46 +52,94 @@ fn key_matches(key: &JSONValue, name: &str) -> bool { /// Find (or create) a child object under `parent`, returning a mutable borrow /// of its key_value_pairs vector. -fn get_or_create_object<'a>( - parent: &'a mut JSONValue, +/// Append a new kvp to `object`, setting whitespace so the output is +/// multi-line with the given indentation: +/// +/// ```text +/// {first_key: first_val,} +/// ``` +/// +/// If `object` already has kvps, the separator between the last one and +/// ours goes in the prior kvp's wsc.3. If we're the first kvp, the +/// lead-in after `{` goes in the object's own wsc.0. +fn append_kvp_pretty( + object: &mut JSONValue, + key: JSONValue, + value: JSONValue, + inner_indent: &str, + outer_indent: &str, +) -> Result<()> { + let (pairs, ctx) = match object { + JSONValue::JSONObject { key_value_pairs, context } => { + let ctx = context.get_or_insert_with(|| JSONObjectContext { + wsc: (String::new(),), + }); + (key_value_pairs, ctx) + } + _ => return Err(anyhow!("not an object")), + }; + + if pairs.is_empty() { + ctx.wsc.0 = format!("\n{}", inner_indent); + } else { + let prev = pairs.last_mut().unwrap(); + let prev_ctx = prev.context.get_or_insert_with(|| KeyValuePairContext { + wsc: (String::new(), String::from(" "), String::new(), None), + }); + prev_ctx.wsc.3 = Some(format!("\n{}", inner_indent)); + } + + pairs.push(JSONKeyValuePair { + key, + value, + context: Some(KeyValuePairContext { + wsc: ( + String::new(), + String::from(" "), + String::new(), + Some(format!("\n{}", outer_indent)), + ), + }), + }); + + Ok(()) +} + +/// Find or create a child object under `parent`. Returns the index of +/// the kvp in parent's key_value_pairs so the caller can re-borrow +/// afterward. +fn get_or_create_object_idx( + parent: &mut JSONValue, section: &str, -) -> Result<&'a mut Vec> { - let pairs = match parent { - JSONValue::JSONObject { key_value_pairs, .. } => key_value_pairs, + inner_indent: &str, + outer_indent: &str, +) -> Result { + let existing = match parent { + JSONValue::JSONObject { key_value_pairs, .. } => { + key_value_pairs.iter() + .position(|kvp| key_matches(&kvp.key, section)) + } _ => return Err(anyhow!("config root is not an object")), }; - // Separate the lookup from the mutable borrow we return — needed to - // satisfy the borrow checker when we create a new entry. - let idx = pairs.iter().position(|kvp| key_matches(&kvp.key, section)); + if let Some(i) = existing { + return Ok(i); + } - let idx = match idx { - Some(i) => i, - None => { - pairs.push(JSONKeyValuePair { - key: JSONValue::Identifier(section.to_string()), - value: JSONValue::JSONObject { - key_value_pairs: Vec::new(), - context: Some(JSONObjectContext { - wsc: (String::new(),), - }), - }, - context: Some(KeyValuePairContext { - wsc: ( - String::from("\n\n "), // whitespace before ':' - String::from(" "), // whitespace after ':' - String::new(), // whitespace after value - Some(String::new()), // whitespace after trailing comma - ), - }), - }); - pairs.len() - 1 - } - }; + append_kvp_pretty( + parent, + JSONValue::Identifier(section.to_string()), + JSONValue::JSONObject { + key_value_pairs: Vec::new(), + context: Some(JSONObjectContext { wsc: (String::new(),) }), + }, + inner_indent, + outer_indent, + )?; - match &mut pairs[idx].value { - JSONValue::JSONObject { key_value_pairs, .. } => Ok(key_value_pairs), - _ => Err(anyhow!("config key '{}' is not an object", section)), + match parent { + JSONValue::JSONObject { key_value_pairs, .. } => Ok(key_value_pairs.len() - 1), + _ => unreachable!(), } } @@ -100,26 +148,36 @@ fn get_or_create_object<'a>( pub fn set_scalar(section: &str, key: &str, literal: &str) -> Result<()> { let value = parse_scalar_literal(literal)?; edit_config(|root| { - let pairs = get_or_create_object(root, section)?; + // New top-level sections sit at column 4 (inside root `{`), + // and the root's closing `}` sits at column 0. + let section_idx = get_or_create_object_idx(root, section, " ", "")?; - if let Some(kvp) = pairs.iter_mut().find(|k| key_matches(&k.key, key)) { - kvp.value = value; - return Ok(()); + let section_value = match root { + JSONValue::JSONObject { key_value_pairs, .. } => { + &mut key_value_pairs[section_idx].value + } + _ => unreachable!(), + }; + + // Update in place if the key already exists. + if let JSONValue::JSONObject { key_value_pairs, .. } = section_value { + if let Some(kvp) = key_value_pairs.iter_mut() + .find(|k| key_matches(&k.key, key)) + { + kvp.value = value; + return Ok(()); + } } - pairs.push(JSONKeyValuePair { - key: JSONValue::Identifier(key.to_string()), + // Append a new kvp. Inner keys sit at column 8, the section's + // closing `}` sits at column 4. + append_kvp_pretty( + section_value, + JSONValue::Identifier(key.to_string()), value, - context: Some(KeyValuePairContext { - wsc: ( - String::from("\n "), - String::from(" "), - String::new(), - Some(String::new()), - ), - }), - }); - Ok(()) + " ", + " ", + ) }) } @@ -166,24 +224,28 @@ mod tests { literal: &str, ) -> Result<()> { let value = parse_scalar_literal(literal)?; - let pairs = get_or_create_object(root, section)?; - if let Some(kvp) = pairs.iter_mut().find(|k| key_matches(&k.key, key)) { - kvp.value = value; - return Ok(()); + let section_idx = get_or_create_object_idx(root, section, " ", "")?; + let section_value = match root { + JSONValue::JSONObject { key_value_pairs, .. } => { + &mut key_value_pairs[section_idx].value + } + _ => unreachable!(), + }; + if let JSONValue::JSONObject { key_value_pairs, .. } = section_value { + if let Some(kvp) = key_value_pairs.iter_mut() + .find(|k| key_matches(&k.key, key)) + { + kvp.value = value; + return Ok(()); + } } - pairs.push(JSONKeyValuePair { - key: JSONValue::Identifier(key.to_string()), + append_kvp_pretty( + section_value, + JSONValue::Identifier(key.to_string()), value, - context: Some(KeyValuePairContext { - wsc: ( - String::from("\n "), - String::from(" "), - String::new(), - Some(String::new()), - ), - }), - }); - Ok(()) + " ", + " ", + ) } fn edit_str Result<()>>(src: &str, f: F) -> Result { @@ -302,7 +364,7 @@ mod tests { assert!(out.contains("1e-7")); // Parse result should parse back without error (real json5 parser). - let reparsed: serde_json::Value = json5::from_str(&out) + let reparsed: serde_json::Value = json_five::from_str(&out) .expect("mutated output must be valid JSON5"); let threshold = reparsed.pointer("/learn/threshold").expect("learn.threshold exists"); assert_eq!(threshold.as_f64(), Some(1e-7)); @@ -324,10 +386,55 @@ mod tests { assert!(!out.contains("0.001")); assert!(out.contains("// The divergence threshold")); - let reparsed: serde_json::Value = json5::from_str(&out).unwrap(); + let reparsed: serde_json::Value = json_five::from_str(&out).unwrap(); assert_eq!(reparsed.pointer("/learn/threshold").and_then(|v| v.as_f64()), Some(5e-8)); } + #[test] + fn new_section_exact_multiline_layout() { + let src = "{\n a: 1,\n}"; + let out = edit_str(src, |root| { + set_scalar_inline(root, "learn", "generate_alternates", "true")?; + set_scalar_inline(root, "learn", "threshold", "1e-7") + }).unwrap(); + + let expected = "\ +{ + a: 1, + learn: { + generate_alternates: true, + threshold: 1e-7, + }, +}"; + assert_eq!(out, expected, "\n--- got ---\n{}\n--- want ---\n{}\n", out, expected); + } + + #[test] + fn new_section_and_key_format_cleanly() { + // The kind of config we actually have in ~/.consciousness + // (top-level sections separated by blank lines, 4-space indent + // for keys within each section). Appending a fresh `learn` + // section with one key should land cleanly, not as + // `learn\n\n :{key\n :value}`. + let src = "{\n memory: {\n user_name: \"Kent\",\n },\n}"; + let out = edit_str(src, |root| { + set_scalar_inline(root, "learn", "generate_alternates", "true") + }).unwrap(); + + // No stray key-to-colon-on-next-line anywhere. + assert!(!out.contains("learn\n"), "learn key wraps: {}", out); + assert!(!out.contains("generate_alternates\n"), + "inner key wraps: {}", out); + + // The output should reparse. + let v: serde_json::Value = json_five::from_str(&out).unwrap(); + assert_eq!( + v.pointer("/learn/generate_alternates").and_then(|x| x.as_bool()), + Some(true), + "output: {}", out, + ); + } + #[test] fn roundtrip_stable_without_change() { let src = r#"{