From 4597c59d7a455549b8460151731cb8e28f1fc41a Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:13:06 +0200 Subject: [PATCH 01/21] refactor: use an enum to represent the KDL version --- src/cli.rs | 18 ++++++------------ src/convert.rs | 20 +++++++++++++------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 18f7f58..1d27a3b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,4 +1,4 @@ -use crate::convert::ConversionError; +use crate::convert::{ConversionError, KdlVersion}; use crate::convert::{convert_and_write_file_content, convert_file_content}; use std::env; use std::path::Path; @@ -39,7 +39,7 @@ pub struct Args { pub output: Option, pub force: bool, pub verbose: bool, - pub kdl_version: i32, + pub kdl_version: KdlVersion, } impl std::fmt::Display for CliError { @@ -79,7 +79,7 @@ impl Args { let mut positional: Vec = vec![]; let mut force = false; let mut verbose = false; - let mut kdl_version = 0; + let mut kdl_version = None; if args.len() == 1 { return Err(CliError::HelpRequested); @@ -93,25 +93,19 @@ impl Args { } else if arg == "-v" || arg == "--verbose" { verbose = true; } else if arg == "-1" || arg == "--kdl-v1" { - if kdl_version != 0 { + if kdl_version.replace(KdlVersion::V1).is_some() { return Err(CliError::MultipleKdlVersion); } - - kdl_version = 1; } else if arg == "-2" || arg == "--kdl-v2" { - if kdl_version != 0 { + if kdl_version.replace(KdlVersion::V2).is_some() { return Err(CliError::MultipleKdlVersion); } - - kdl_version = 2; } else if arg == "-h" || arg == "--help" { return Err(CliError::HelpRequested); } } - if kdl_version == 0 { - kdl_version = 2; - }; + let kdl_version = kdl_version.unwrap_or_default(); let input = positional.get(0).ok_or(CliError::MissingInput)?.to_string(); diff --git a/src/convert.rs b/src/convert.rs index c55befc..e8764a9 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -43,7 +43,14 @@ impl From for ConversionError { pub type Result = std::result::Result; -pub fn convert_file_content(input: &Path, kdl_version: i32) -> Result { +#[derive(Clone, Copy, Debug, Eq, PartialEq, Default)] +pub enum KdlVersion { + V1, + #[default] + V2, +} + +pub fn convert_file_content(input: &Path, version: KdlVersion) -> Result { let json_content = fs::read_to_string(input)?; let json_value: Value = serde_json::from_str(&json_content)?; let mut kdl_doc = json_to_kdl(json_value)?; @@ -51,10 +58,9 @@ pub fn convert_file_content(input: &Path, kdl_version: i32) -> Result { // For some reason, you MUST autoformat before ensuring version. kdl_doc.autoformat(); - if kdl_version == 2 { - kdl_doc.ensure_v2(); - } else { - kdl_doc.ensure_v1(); + match version { + KdlVersion::V1 => kdl_doc.ensure_v1(), + KdlVersion::V2 => kdl_doc.ensure_v2(), } Ok(kdl_doc.to_string()) @@ -64,9 +70,9 @@ pub fn convert_and_write_file_content( input: &Path, output: &Path, verbose: bool, - kdl_version: i32, + version: KdlVersion, ) -> Result<()> { - let kdl_doc_content = convert_file_content(input, kdl_version)?; + let kdl_doc_content = convert_file_content(input, version)?; fs::write(output, kdl_doc_content)?; From a032147f46c0a06e645879877e594de61eaa5adf Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:19:43 +0200 Subject: [PATCH 02/21] refactor: call it `json: JsonValue` throughout, contrasting with `Kdl*` types --- src/convert.rs | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index e8764a9..3cb18c1 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -1,5 +1,5 @@ use kdl::{KdlDocument, KdlEntry, KdlNode, KdlValue, NodeKey}; -use serde_json::Value; +use serde_json::Value as JsonValue; use std::{fmt, fs, path::Path}; #[derive(Debug)] @@ -52,7 +52,7 @@ pub enum KdlVersion { pub fn convert_file_content(input: &Path, version: KdlVersion) -> Result { let json_content = fs::read_to_string(input)?; - let json_value: Value = serde_json::from_str(&json_content)?; + let json_value: JsonValue = serde_json::from_str(&json_content)?; let mut kdl_doc = json_to_kdl(json_value)?; // For some reason, you MUST autoformat before ensuring version. @@ -83,7 +83,7 @@ pub fn convert_and_write_file_content( Ok(()) } -pub fn json_to_kdl(json: Value) -> Result { +pub fn json_to_kdl(json: JsonValue) -> Result { let array = json.as_array().ok_or_else(|| { ConversionError::InvalidStructure("document root must be json array".to_string()) })?; @@ -98,15 +98,15 @@ pub fn json_to_kdl(json: Value) -> Result { Ok(document) } -fn json_value_to_node(value: &Value) -> Result { - let name = value.get("name").and_then(|n| n.as_str()).ok_or_else(|| { +fn json_value_to_node(json: &JsonValue) -> Result { + let name = json.get("name").and_then(|n| n.as_str()).ok_or_else(|| { ConversionError::InvalidStructure("name must be non-empty string".to_string()) })?; let mut node = KdlNode::new(name); // Handle arguments - if let Some(arguments) = value.get("arguments") { + if let Some(arguments) = json.get("arguments") { let args = arguments.as_array().ok_or_else(|| { ConversionError::InvalidStructure("arguments must be an array".to_string()) })?; @@ -118,7 +118,7 @@ fn json_value_to_node(value: &Value) -> Result { } // Handle properties - if let Some(properties) = value.get("properties") { + if let Some(properties) = json.get("properties") { let props = properties.as_object().ok_or_else(|| { ConversionError::InvalidStructure("properties must be an object".to_string()) })?; @@ -130,13 +130,13 @@ fn json_value_to_node(value: &Value) -> Result { } // Handle children - if let Some(children) = value.get("children") { + if let Some(children) = json.get("children") { let child_doc = json_to_kdl(children.clone())?; node.set_children(child_doc); } // Handle type annotation - if let Some(type_value) = value.get("type") { + if let Some(type_value) = json.get("type") { if !type_value.is_null() { if let Some(type_str) = type_value.as_str() { node.set_ty(type_str); @@ -147,22 +147,22 @@ fn json_value_to_node(value: &Value) -> Result { Ok(node) } -fn json_value_to_entry(value: &Value) -> Result { - let (actual_value, type_annotation) = if let Some(obj) = value.as_object() { - let val = obj.get("value").unwrap_or(value); +fn json_value_to_entry(json: &JsonValue) -> Result { + let (actual_value, type_annotation) = if let Some(obj) = json.as_object() { + let val = obj.get("value").unwrap_or(json); let ty = obj .get("type") .and_then(|t| t.as_str()) .map(|s| s.to_string()); (val, ty) } else { - (value, None) + (json, None) }; let kdl_value = match actual_value { - Value::Null => KdlValue::Null, - Value::Bool(b) => KdlValue::Bool(*b), - Value::Number(n) => { + JsonValue::Null => KdlValue::Null, + JsonValue::Bool(b) => KdlValue::Bool(*b), + JsonValue::Number(n) => { if let Some(i) = n.as_i64() { KdlValue::Integer(i as i128) } else if let Some(f) = n.as_f64() { @@ -173,7 +173,7 @@ fn json_value_to_entry(value: &Value) -> Result { )); } } - Value::String(s) => KdlValue::String(s.clone()), + JsonValue::String(s) => KdlValue::String(s.clone()), _ => { return Err(ConversionError::InvalidStructure( "unsupported json value type for kdl conversion".to_string(), From 5104d2f32289193bf8d2d3d20ba324fd51a896f7 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:22:18 +0200 Subject: [PATCH 03/21] refactor: consistently use `convert_*` naming of functions --- src/convert.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 3cb18c1..f587377 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -53,7 +53,7 @@ pub enum KdlVersion { pub fn convert_file_content(input: &Path, version: KdlVersion) -> Result { let json_content = fs::read_to_string(input)?; let json_value: JsonValue = serde_json::from_str(&json_content)?; - let mut kdl_doc = json_to_kdl(json_value)?; + let mut kdl_doc = convert_document(json_value)?; // For some reason, you MUST autoformat before ensuring version. kdl_doc.autoformat(); @@ -83,7 +83,7 @@ pub fn convert_and_write_file_content( Ok(()) } -pub fn json_to_kdl(json: JsonValue) -> Result { +pub fn convert_document(json: JsonValue) -> Result { let array = json.as_array().ok_or_else(|| { ConversionError::InvalidStructure("document root must be json array".to_string()) })?; @@ -91,14 +91,14 @@ pub fn json_to_kdl(json: JsonValue) -> Result { let mut document = KdlDocument::new(); for value in array { - let node = json_value_to_node(value)?; + let node = convert_node(value)?; document.nodes_mut().push(node); } Ok(document) } -fn json_value_to_node(json: &JsonValue) -> Result { +fn convert_node(json: &JsonValue) -> Result { let name = json.get("name").and_then(|n| n.as_str()).ok_or_else(|| { ConversionError::InvalidStructure("name must be non-empty string".to_string()) })?; @@ -112,7 +112,7 @@ fn json_value_to_node(json: &JsonValue) -> Result { })?; for arg in args { - let entry = json_value_to_entry(arg)?; + let entry = convert_entry(arg)?; node.push(entry); } } @@ -124,14 +124,14 @@ fn json_value_to_node(json: &JsonValue) -> Result { })?; for (key, prop_value) in props { - let entry = json_value_to_entry(prop_value)?; + let entry = convert_entry(prop_value)?; node.insert(NodeKey::from(key.clone()), entry); } } // Handle children if let Some(children) = json.get("children") { - let child_doc = json_to_kdl(children.clone())?; + let child_doc = convert_document(children.clone())?; node.set_children(child_doc); } @@ -147,7 +147,7 @@ fn json_value_to_node(json: &JsonValue) -> Result { Ok(node) } -fn json_value_to_entry(json: &JsonValue) -> Result { +fn convert_entry(json: &JsonValue) -> Result { let (actual_value, type_annotation) = if let Some(obj) = json.as_object() { let val = obj.get("value").unwrap_or(json); let ty = obj From 9b02800ddb580bb5783879a5c7c372acdd1004db Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:24:05 +0200 Subject: [PATCH 04/21] refactor: make convert_document accept reference one less `clone()` yay --- src/convert.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index f587377..8fefd70 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -53,7 +53,7 @@ pub enum KdlVersion { pub fn convert_file_content(input: &Path, version: KdlVersion) -> Result { let json_content = fs::read_to_string(input)?; let json_value: JsonValue = serde_json::from_str(&json_content)?; - let mut kdl_doc = convert_document(json_value)?; + let mut kdl_doc = convert_document(&json_value)?; // For some reason, you MUST autoformat before ensuring version. kdl_doc.autoformat(); @@ -83,7 +83,7 @@ pub fn convert_and_write_file_content( Ok(()) } -pub fn convert_document(json: JsonValue) -> Result { +pub fn convert_document(json: &JsonValue) -> Result { let array = json.as_array().ok_or_else(|| { ConversionError::InvalidStructure("document root must be json array".to_string()) })?; @@ -131,7 +131,7 @@ fn convert_node(json: &JsonValue) -> Result { // Handle children if let Some(children) = json.get("children") { - let child_doc = convert_document(children.clone())?; + let child_doc = convert_document(children)?; node.set_children(child_doc); } From 976bf330bdb1e9d536f556f564392b8804d551dc Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:45:35 +0200 Subject: [PATCH 05/21] refactor: rename `kdl_doc` -> `document` (match `convert_node`) --- src/convert.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 8fefd70..30255bd 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -53,17 +53,18 @@ pub enum KdlVersion { pub fn convert_file_content(input: &Path, version: KdlVersion) -> Result { let json_content = fs::read_to_string(input)?; let json_value: JsonValue = serde_json::from_str(&json_content)?; - let mut kdl_doc = convert_document(&json_value)?; + + let mut document = convert_document(&json_value)?; // For some reason, you MUST autoformat before ensuring version. - kdl_doc.autoformat(); + document.autoformat(); match version { - KdlVersion::V1 => kdl_doc.ensure_v1(), - KdlVersion::V2 => kdl_doc.ensure_v2(), + KdlVersion::V1 => document.ensure_v1(), + KdlVersion::V2 => document.ensure_v2(), } - Ok(kdl_doc.to_string()) + Ok(document.to_string()) } pub fn convert_and_write_file_content( From 46b75e61615f55c088d0bc95960309254f3a8481 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:36:12 +0200 Subject: [PATCH 06/21] refactor: shadow names after they've been refined --- src/convert.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 30255bd..324bfdc 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -85,13 +85,13 @@ pub fn convert_and_write_file_content( } pub fn convert_document(json: &JsonValue) -> Result { - let array = json.as_array().ok_or_else(|| { + let json = json.as_array().ok_or_else(|| { ConversionError::InvalidStructure("document root must be json array".to_string()) })?; let mut document = KdlDocument::new(); - for value in array { + for value in json { let node = convert_node(value)?; document.nodes_mut().push(node); } @@ -108,11 +108,11 @@ fn convert_node(json: &JsonValue) -> Result { // Handle arguments if let Some(arguments) = json.get("arguments") { - let args = arguments.as_array().ok_or_else(|| { + let arguments = arguments.as_array().ok_or_else(|| { ConversionError::InvalidStructure("arguments must be an array".to_string()) })?; - for arg in args { + for arg in arguments { let entry = convert_entry(arg)?; node.push(entry); } @@ -120,11 +120,11 @@ fn convert_node(json: &JsonValue) -> Result { // Handle properties if let Some(properties) = json.get("properties") { - let props = properties.as_object().ok_or_else(|| { + let properties = properties.as_object().ok_or_else(|| { ConversionError::InvalidStructure("properties must be an object".to_string()) })?; - for (key, prop_value) in props { + for (key, prop_value) in properties { let entry = convert_entry(prop_value)?; node.insert(NodeKey::from(key.clone()), entry); } @@ -132,8 +132,8 @@ fn convert_node(json: &JsonValue) -> Result { // Handle children if let Some(children) = json.get("children") { - let child_doc = convert_document(children)?; - node.set_children(child_doc); + let children = convert_document(children)?; + node.set_children(children); } // Handle type annotation From 2107289a257c516f1f33e9b71d01b325111a3d1a Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:45:55 +0200 Subject: [PATCH 07/21] refactor: extract `convert_value` --- src/convert.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 324bfdc..68da148 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -160,7 +160,18 @@ fn convert_entry(json: &JsonValue) -> Result { (json, None) }; - let kdl_value = match actual_value { + let kdl_value = convert_value(actual_value)?; + + let mut entry = KdlEntry::new(kdl_value); + if let Some(ty) = type_annotation { + entry.set_ty(ty); + } + + Ok(entry) +} + +fn convert_value(json: &JsonValue) -> Result { + Ok(match json { JsonValue::Null => KdlValue::Null, JsonValue::Bool(b) => KdlValue::Bool(*b), JsonValue::Number(n) => { @@ -180,12 +191,5 @@ fn convert_entry(json: &JsonValue) -> Result { "unsupported json value type for kdl conversion".to_string(), )); } - }; - - let mut entry = KdlEntry::new(kdl_value); - if let Some(ty) = type_annotation { - entry.set_ty(ty); - } - - Ok(entry) + }) } From d5c48f881c35a4b015ad8242b5c3f28d0dcbfe24 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:06:10 +0200 Subject: [PATCH 08/21] refactor: don't wrap `convert_value` in an `Ok()` --- src/convert.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 68da148..b140d28 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -171,25 +171,23 @@ fn convert_entry(json: &JsonValue) -> Result { } fn convert_value(json: &JsonValue) -> Result { - Ok(match json { - JsonValue::Null => KdlValue::Null, - JsonValue::Bool(b) => KdlValue::Bool(*b), + match json { + JsonValue::Null => Ok(KdlValue::Null), + JsonValue::Bool(b) => Ok(KdlValue::Bool(*b)), JsonValue::Number(n) => { if let Some(i) = n.as_i64() { - KdlValue::Integer(i as i128) + Ok(KdlValue::Integer(i as i128)) } else if let Some(f) = n.as_f64() { - KdlValue::Float(f) + Ok(KdlValue::Float(f)) } else { - return Err(ConversionError::InvalidStructure( + Err(ConversionError::InvalidStructure( "invalid number value".to_string(), - )); + )) } } - JsonValue::String(s) => KdlValue::String(s.clone()), - _ => { - return Err(ConversionError::InvalidStructure( - "unsupported json value type for kdl conversion".to_string(), - )); - } - }) + JsonValue::String(s) => Ok(KdlValue::String(s.clone())), + _ => Err(ConversionError::InvalidStructure( + "unsupported json value type for kdl conversion".to_string(), + )), + } } From e63ecbd13248c33bb38b7802a973c1441e29efcb Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 21:51:01 +0200 Subject: [PATCH 09/21] refactor: make `convert_entry` handle type like `convert_node` it behaves identically; just it's nicer to read imo --- src/convert.rs | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index b140d28..84f9fdb 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -149,22 +149,16 @@ fn convert_node(json: &JsonValue) -> Result { } fn convert_entry(json: &JsonValue) -> Result { - let (actual_value, type_annotation) = if let Some(obj) = json.as_object() { - let val = obj.get("value").unwrap_or(json); - let ty = obj - .get("type") - .and_then(|t| t.as_str()) - .map(|s| s.to_string()); - (val, ty) - } else { - (json, None) - }; - - let kdl_value = convert_value(actual_value)?; - - let mut entry = KdlEntry::new(kdl_value); - if let Some(ty) = type_annotation { - entry.set_ty(ty); + let value = convert_value(json.get("value").unwrap_or(json))?; + + let mut entry = KdlEntry::new(value); + + if let Some(type_value) = json.get("type") { + if !type_value.is_null() { + if let Some(type_str) = type_value.as_str() { + entry.set_ty(type_str); + } + } } Ok(entry) From 06f3058f95b20c8a72f0423880f99e276067f976 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:15:28 +0200 Subject: [PATCH 10/21] refactor: extract `convert_type` --- src/convert.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 84f9fdb..42dd603 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -1,4 +1,4 @@ -use kdl::{KdlDocument, KdlEntry, KdlNode, KdlValue, NodeKey}; +use kdl::{KdlDocument, KdlEntry, KdlIdentifier, KdlNode, KdlValue, NodeKey}; use serde_json::Value as JsonValue; use std::{fmt, fs, path::Path}; @@ -137,11 +137,9 @@ fn convert_node(json: &JsonValue) -> Result { } // Handle type annotation - if let Some(type_value) = json.get("type") { - if !type_value.is_null() { - if let Some(type_str) = type_value.as_str() { - node.set_ty(type_str); - } + if let Some(ty) = json.get("type") { + if let Some(ty) = convert_type(ty)? { + node.set_ty(ty); } } @@ -153,11 +151,9 @@ fn convert_entry(json: &JsonValue) -> Result { let mut entry = KdlEntry::new(value); - if let Some(type_value) = json.get("type") { - if !type_value.is_null() { - if let Some(type_str) = type_value.as_str() { - entry.set_ty(type_str); - } + if let Some(ty) = json.get("type") { + if let Some(ty) = convert_type(ty)? { + entry.set_ty(ty); } } @@ -185,3 +181,12 @@ fn convert_value(json: &JsonValue) -> Result { )), } } + +fn convert_type(json: &JsonValue) -> Result> { + if !json.is_null() { + if let Some(type_str) = json.as_str() { + return Ok(Some(KdlIdentifier::from(type_str))); + } + } + Ok(None) +} From a54802ea94ba9f3273b7b3e8f4d886225a99814b Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:41:28 +0200 Subject: [PATCH 11/21] refactor: use `entry.set_name` when converting properties `node.insert` pulls in a bunch of logic about not having duplicate keys; but that's already guaranteed by a json map, so there's no reason to do that again in our code. this is simpler, faster, and smaller (yay! premature optimization!) --- src/convert.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 42dd603..9183204 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -1,4 +1,4 @@ -use kdl::{KdlDocument, KdlEntry, KdlIdentifier, KdlNode, KdlValue, NodeKey}; +use kdl::{KdlDocument, KdlEntry, KdlIdentifier, KdlNode, KdlValue}; use serde_json::Value as JsonValue; use std::{fmt, fs, path::Path}; @@ -125,8 +125,9 @@ fn convert_node(json: &JsonValue) -> Result { })?; for (key, prop_value) in properties { - let entry = convert_entry(prop_value)?; - node.insert(NodeKey::from(key.clone()), entry); + let mut entry = convert_entry(prop_value)?; + entry.set_name(Some(key.as_str())); + node.push(entry); } } From 798b434db6d4fefa2d7498c07d4f4c79295c53cb Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:30:01 +0200 Subject: [PATCH 12/21] fix: don't accept invalidly-typed `type` value --- src/convert.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 9183204..4801bea 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -184,10 +184,11 @@ fn convert_value(json: &JsonValue) -> Result { } fn convert_type(json: &JsonValue) -> Result> { - if !json.is_null() { - if let Some(type_str) = json.as_str() { - return Ok(Some(KdlIdentifier::from(type_str))); - } + match json { + JsonValue::String(ty) => Ok(Some(KdlIdentifier::from(ty.as_str()))), + JsonValue::Null => Ok(None), + _ => Err(ConversionError::InvalidStructure( + "type must be a string or null".to_string(), + )), } - Ok(None) } From d117c517f9f56a1b07eface898a6546bb2ae71c6 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:33:36 +0200 Subject: [PATCH 13/21] fix: more accurate error message when `name` isn't a string names can be empty; and we never did check that they're nonempty --- src/convert.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 4801bea..048729c 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -100,9 +100,10 @@ pub fn convert_document(json: &JsonValue) -> Result { } fn convert_node(json: &JsonValue) -> Result { - let name = json.get("name").and_then(|n| n.as_str()).ok_or_else(|| { - ConversionError::InvalidStructure("name must be non-empty string".to_string()) - })?; + let name = json + .get("name") + .and_then(|n| n.as_str()) + .ok_or_else(|| ConversionError::InvalidStructure("name must be string".to_string()))?; let mut node = KdlNode::new(name); From c255bb72950bbca3457453444a50d59cc934c507 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:47:09 +0200 Subject: [PATCH 14/21] fix: more consistent grammar in some error messages "an array" over "json array" to match "arguments must be an array" --- src/convert.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 048729c..3c22111 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -86,7 +86,7 @@ pub fn convert_and_write_file_content( pub fn convert_document(json: &JsonValue) -> Result { let json = json.as_array().ok_or_else(|| { - ConversionError::InvalidStructure("document root must be json array".to_string()) + ConversionError::InvalidStructure("document root must be an array".to_string()) })?; let mut document = KdlDocument::new(); @@ -103,7 +103,7 @@ fn convert_node(json: &JsonValue) -> Result { let name = json .get("name") .and_then(|n| n.as_str()) - .ok_or_else(|| ConversionError::InvalidStructure("name must be string".to_string()))?; + .ok_or_else(|| ConversionError::InvalidStructure("name must be a string".to_string()))?; let mut node = KdlNode::new(name); From 1a94eea6ea3c5e151516a734ee2dc10acf6e1c59 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:53:41 +0200 Subject: [PATCH 15/21] fix: more relevant error message when `node` isn't an object previously, this would throw "name must be a string"; since name field is obligatory. --- src/convert.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/convert.rs b/src/convert.rs index 3c22111..21ae2ca 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -100,6 +100,10 @@ pub fn convert_document(json: &JsonValue) -> Result { } fn convert_node(json: &JsonValue) -> Result { + let json = json + .as_object() + .ok_or_else(|| ConversionError::InvalidStructure("node must be an object".to_string()))?; + let name = json .get("name") .and_then(|n| n.as_str()) From c31beb46115057c60ecab25a1fabf419f78daf95 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 22:59:15 +0200 Subject: [PATCH 16/21] fix: more relevant error message when `name` is missing --- src/convert.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 21ae2ca..11d7771 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -104,10 +104,15 @@ fn convert_node(json: &JsonValue) -> Result { .as_object() .ok_or_else(|| ConversionError::InvalidStructure("node must be an object".to_string()))?; - let name = json - .get("name") - .and_then(|n| n.as_str()) - .ok_or_else(|| ConversionError::InvalidStructure("name must be a string".to_string()))?; + let name = match json.get("name") { + Some(JsonValue::String(name)) => Ok(name.as_str()), + Some(_) => Err(ConversionError::InvalidStructure( + "name must be a string".to_string(), + )), + None => Err(ConversionError::InvalidStructure( + "node must have a name".to_string(), + )), + }?; let mut node = KdlNode::new(name); From 1815a90312cdc50e676e567f93b36dd694deeae4 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 23:18:47 +0200 Subject: [PATCH 17/21] fix: don't silently ignore unknown command-line options --- src/cli.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index 1d27a3b..5db0baa 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -25,6 +25,7 @@ pub enum CliError { MissingInput, HelpRequested, MultipleKdlVersion, + UnknownOption(String), InvalidInputPath(String), FileExists(String), InputNotFound(String), @@ -48,6 +49,7 @@ impl std::fmt::Display for CliError { CliError::MissingInput => writeln!(f, "missing input path"), CliError::HelpRequested => writeln!(f, "help requested"), CliError::MultipleKdlVersion => writeln!(f, "specify only one of --kdl-v1 or --kdl-v2"), + CliError::UnknownOption(opt) => writeln!(f, "unknown command-line option {opt}"), CliError::InvalidInputPath(path) => writeln!(f, "not a file: {}", path), CliError::FileExists(path) => { writeln!(f, "file exists: {} (use --force to overwrite)", path) @@ -102,6 +104,8 @@ impl Args { } } else if arg == "-h" || arg == "--help" { return Err(CliError::HelpRequested); + } else { + return Err(CliError::UnknownOption(arg.into())); } } From 24b322d6d34ed60820d2a766683bf1363f58233c Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 23:21:41 +0200 Subject: [PATCH 18/21] fix: don't silently ignore additional positional arguments --- src/cli.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index 5db0baa..ad7d1a3 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -26,6 +26,7 @@ pub enum CliError { HelpRequested, MultipleKdlVersion, UnknownOption(String), + TooManyPositionals, InvalidInputPath(String), FileExists(String), InputNotFound(String), @@ -50,6 +51,7 @@ impl std::fmt::Display for CliError { CliError::HelpRequested => writeln!(f, "help requested"), CliError::MultipleKdlVersion => writeln!(f, "specify only one of --kdl-v1 or --kdl-v2"), CliError::UnknownOption(opt) => writeln!(f, "unknown command-line option {opt}"), + CliError::TooManyPositionals => writeln!(f, "too many positional arguments"), CliError::InvalidInputPath(path) => writeln!(f, "not a file: {}", path), CliError::FileExists(path) => { writeln!(f, "file exists: {} (use --force to overwrite)", path) @@ -115,6 +117,10 @@ impl Args { let output = positional.get(1).map(|s| s.to_string()); + if positional.len() > 2 { + return Err(CliError::TooManyPositionals); + } + let result = Self { input, output, From 0078f07a60602399e2c0ba2d31032c15dc342671 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Wed, 23 Jul 2025 23:47:38 +0200 Subject: [PATCH 19/21] feat: accept `--` for positionals starting with `-` and non-Unicode because options now canonically come "before" the positional arguments, i also flipped the help text --- src/cli.rs | 135 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 85 insertions(+), 50 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index ad7d1a3..666895a 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,23 +1,24 @@ use crate::convert::{ConversionError, KdlVersion}; use crate::convert::{convert_and_write_file_content, convert_file_content}; use std::env; -use std::path::Path; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; const HELP_TEXT: &str = "\ -Usage: jsonkdl [options] +Usage: jsonkdl [options] [--] Converts JSON to KDL. By default, KDL spec v2 is used. -Arguments: - Path to input JSON file - Path to output KDL file - Options: -1, --kdl-v1 Convert to KDL v1 -2, --kdl-v2 Convert to KDL v2 -f, --force Overwrite output if it exists -v, --verbose Print extra information during conversion -h, --help Show this help message + +Arguments: + Path to input JSON file + Path to output KDL file "; #[derive(Debug)] @@ -27,9 +28,10 @@ pub enum CliError { MultipleKdlVersion, UnknownOption(String), TooManyPositionals, - InvalidInputPath(String), - FileExists(String), - InputNotFound(String), + NotUnicode(OsString), + InvalidInputPath(PathBuf), + FileExists(PathBuf), + InputNotFound(PathBuf), Conversion(ConversionError), } @@ -37,8 +39,8 @@ pub type Result = std::result::Result; #[derive(Debug)] pub struct Args { - pub input: String, - pub output: Option, + pub input: PathBuf, + pub output: Option, pub force: bool, pub verbose: bool, pub kdl_version: KdlVersion, @@ -50,13 +52,24 @@ impl std::fmt::Display for CliError { CliError::MissingInput => writeln!(f, "missing input path"), CliError::HelpRequested => writeln!(f, "help requested"), CliError::MultipleKdlVersion => writeln!(f, "specify only one of --kdl-v1 or --kdl-v2"), - CliError::UnknownOption(opt) => writeln!(f, "unknown command-line option {opt}"), + CliError::UnknownOption(opt) => writeln!( + f, + "unknown command-line option {opt} (use `--` to pass arbitrary filenames)" + ), CliError::TooManyPositionals => writeln!(f, "too many positional arguments"), - CliError::InvalidInputPath(path) => writeln!(f, "not a file: {}", path), + CliError::NotUnicode(arg) => writeln!( + f, + "the argument {arg:?} was not valid Unicode (use `--` to pass arbitrary filenames)" + ), + CliError::InvalidInputPath(path) => writeln!(f, "not a file: {}", path.display()), CliError::FileExists(path) => { - writeln!(f, "file exists: {} (use --force to overwrite)", path) + writeln!( + f, + "file exists: {} (use --force to overwrite)", + path.display() + ) } - CliError::InputNotFound(path) => writeln!(f, "no such file: {}", path), + CliError::InputNotFound(path) => writeln!(f, "no such file: {}", path.display()), CliError::Conversion(e) => writeln!(f, "conversion error: {}", e), } } @@ -79,47 +92,71 @@ impl From for CliError { impl Args { fn parse() -> Result { - let args: Vec = env::args().collect(); - let mut positional: Vec = vec![]; - let mut force = false; - let mut verbose = false; - let mut kdl_version = None; + let args = env::args_os(); if args.len() == 1 { return Err(CliError::HelpRequested); } - for arg in args.iter().skip(1) { - if !arg.starts_with('-') { - positional.push(arg.into()); - } else if arg == "-f" || arg == "--force" { - force = true; - } else if arg == "-v" || arg == "--verbose" { - verbose = true; - } else if arg == "-1" || arg == "--kdl-v1" { - if kdl_version.replace(KdlVersion::V1).is_some() { - return Err(CliError::MultipleKdlVersion); + let args = args.skip(1); + + let mut force = false; + let mut verbose = false; + let mut kdl_version = None; + + let mut positionals_only = false; + + let mut input = None; + let mut output = None; + + for arg in args { + let is_positional; + + if positionals_only { + is_positional = true; + } else { + let Some(arg) = arg.to_str() else { + return Err(CliError::NotUnicode(arg)); + }; + + if arg.starts_with("-") { + is_positional = false; + match arg { + "--" => positionals_only = true, + "-f" | "--force" => force = true, + "-v" | "--verbose" => verbose = true, + "-1" | "--kdl-v1" => { + if kdl_version.replace(KdlVersion::V1).is_some() { + return Err(CliError::MultipleKdlVersion); + } + } + "-2" | "--kdl-v2" => { + if kdl_version.replace(KdlVersion::V2).is_some() { + return Err(CliError::MultipleKdlVersion); + } + } + "-h" | "--help" => return Err(CliError::HelpRequested), + _ => return Err(CliError::UnknownOption(arg.to_string())), + } + } else { + is_positional = true; } - } else if arg == "-2" || arg == "--kdl-v2" { - if kdl_version.replace(KdlVersion::V2).is_some() { - return Err(CliError::MultipleKdlVersion); + } + + if is_positional { + if input.is_none() { + input = Some(PathBuf::from(arg)); + } else if output.is_none() { + output = Some(PathBuf::from(arg)); + } else { + return Err(CliError::TooManyPositionals); } - } else if arg == "-h" || arg == "--help" { - return Err(CliError::HelpRequested); - } else { - return Err(CliError::UnknownOption(arg.into())); } } let kdl_version = kdl_version.unwrap_or_default(); - let input = positional.get(0).ok_or(CliError::MissingInput)?.to_string(); - - let output = positional.get(1).map(|s| s.to_string()); - - if positional.len() > 2 { - return Err(CliError::TooManyPositionals); - } + let input = input.ok_or(CliError::MissingInput)?; let result = Self { input, @@ -147,13 +184,11 @@ pub fn run() -> Result<()> { Err(e) => return Err(e), }; - let input_path = Path::new(&args.input); - - if !input_path.exists() { + if !args.input.exists() { return Err(CliError::InputNotFound(args.input)); } - if !input_path.is_file() { + if !args.input.is_file() { return Err(CliError::InvalidInputPath(args.input)); } @@ -164,9 +199,9 @@ pub fn run() -> Result<()> { return Err(CliError::FileExists(output)); } - convert_and_write_file_content(input_path, output_path, args.verbose, args.kdl_version)?; + convert_and_write_file_content(&args.input, output_path, args.verbose, args.kdl_version)?; } else { - let kdl_content = convert_file_content(input_path, args.kdl_version)?; + let kdl_content = convert_file_content(&args.input, args.kdl_version)?; println!("{}", kdl_content); } From e989d2ae38b2558bd6bfc68ffcb79585427d9f5b Mon Sep 17 00:00:00 2001 From: sodiboo Date: Thu, 24 Jul 2025 00:12:44 +0200 Subject: [PATCH 20/21] fix: document that the output file is optional --- src/cli.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 666895a..36c66b7 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -5,7 +5,7 @@ use std::ffi::OsString; use std::path::{Path, PathBuf}; const HELP_TEXT: &str = "\ -Usage: jsonkdl [options] [--] +Usage: jsonkdl [options] [--] [] Converts JSON to KDL. By default, KDL spec v2 is used. @@ -18,7 +18,9 @@ Options: Arguments: Path to input JSON file - Path to output KDL file + Path to output KDL file (optional) + +If no output file is provided, the resulting KDL document is printed to standard output. "; #[derive(Debug)] From 76fca431170af7d15f75b8b05fdf5a2a076cf200 Mon Sep 17 00:00:00 2001 From: sodiboo Date: Thu, 24 Jul 2025 00:15:34 +0200 Subject: [PATCH 21/21] refactor: document kdl version default more concisely --- src/cli.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 36c66b7..e1f7117 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -7,11 +7,10 @@ use std::path::{Path, PathBuf}; const HELP_TEXT: &str = "\ Usage: jsonkdl [options] [--] [] Converts JSON to KDL. -By default, KDL spec v2 is used. Options: -1, --kdl-v1 Convert to KDL v1 - -2, --kdl-v2 Convert to KDL v2 + -2, --kdl-v2 Convert to KDL v2 (default) -f, --force Overwrite output if it exists -v, --verbose Print extra information during conversion -h, --help Show this help message