From f49a7c9eaa14ce1aaf342d862ec292b84fa2e7bc Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Fri, 21 Jan 2022 09:34:47 +0200 Subject: refactor: parse with serde directly to data structure Use serde to parse directly to a hashmap of steps, instead of generic YAML values. This gives us better error messages. Sponsored-by: author --- Cargo.lock | 15 +++++++++ Cargo.toml | 1 + src/map.rs | 18 +++++++---- src/parser.rs | 101 +++++----------------------------------------------------- src/status.rs | 11 ++++++- src/step.rs | 15 +++++++-- 6 files changed, 58 insertions(+), 103 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 87e468d..be68abe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -186,6 +186,7 @@ name = "roadmap" version = "0.3.0" dependencies = [ "anyhow", + "serde", "serde_yaml", "structopt", "textwrap 0.14.2", @@ -203,6 +204,20 @@ name = "serde" version = "1.0.134" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "96b3c34c1690edf8174f5b289a336ab03f568a4460d8c6df75f2f3a692b3bc6a" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.134" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "784ed1fbfa13fe191077537b0d70ec8ad1e903cfe04831da608aa36457cb653d" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] [[package]] name = "serde_yaml" diff --git a/Cargo.toml b/Cargo.toml index a5f1129..40ef824 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,6 +11,7 @@ repostiory = "https://gitlab.com/larswirzenius/roadmap" [dependencies] anyhow = "1" +serde = { version = "1.0.134", features = ["derive"] } serde_yaml = "0.8" structopt = "0.3" textwrap = "0.14" diff --git a/src/map.rs b/src/map.rs index e6a24a9..b592fbd 100644 --- a/src/map.rs +++ b/src/map.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use textwrap::fill; pub use crate::from_yaml; @@ -21,8 +23,10 @@ impl Roadmap { /// Create a new, empty roadmap. /// /// You probably want the `from_yaml` function instead. - pub fn new() -> Self { - Self::default() + pub fn new(map: HashMap) -> Self { + Self { + steps: map.values().cloned().collect(), + } } // Find steps that nothing depends on. @@ -220,13 +224,13 @@ mod tests { #[test] fn new_roadmap() { - let roadmap = Roadmap::new(); + let roadmap = Roadmap::default(); assert_eq!(roadmap.step_names().count(), 0); } #[test] fn add_step_to_roadmap() { - let mut roadmap = Roadmap::new(); + let mut roadmap = Roadmap::default(); let first = Step::new("first", "the first step"); roadmap.add_step(first); let names: Vec<&str> = roadmap.step_names().collect(); @@ -235,7 +239,7 @@ mod tests { #[test] fn get_step_from_roadmap() { - let mut roadmap = Roadmap::new(); + let mut roadmap = Roadmap::default(); let first = Step::new("first", "the first step"); roadmap.add_step(first); let gotit = roadmap.get_step("first").unwrap(); @@ -279,7 +283,7 @@ blocked: #[test] fn empty_dot() { - let roadmap = Roadmap::new(); + let roadmap = Roadmap::default(); match roadmap.format_as_dot(999) { Err(_) => (), _ => panic!("expected error for empty roadmap"), @@ -288,7 +292,7 @@ blocked: #[test] fn simple_dot() { - let mut roadmap = Roadmap::new(); + let mut roadmap = Roadmap::default(); let mut first = Step::new("first", ""); first.set_status(Status::Ready); let mut second = Step::new("second", ""); diff --git a/src/parser.rs b/src/parser.rs index 6cc3d25..8c7a9c1 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1,4 +1,3 @@ -use serde_yaml::Value; use std::collections::HashMap; pub use crate::Roadmap; @@ -9,121 +8,37 @@ pub use crate::Step; /// Create a new roadmap from a textual YAML representation. pub fn from_yaml(yaml: &str) -> RoadmapResult { - let mut roadmap = Roadmap::new(); - let map: HashMap = serde_yaml::from_str(yaml)?; - - for (name, value) in map { - let step = step_from_value(&name, &value)?; - roadmap.add_step(step); + let mut roadmap: HashMap = serde_yaml::from_str(yaml)?; + for (name, step) in roadmap.iter_mut() { + step.set_name(name); } - + let roadmap = Roadmap::new(roadmap); roadmap.validate()?; Ok(roadmap) } -// Convert a Value into a Step, if possible. -fn step_from_value(name: &str, value: &Value) -> RoadmapResult { - match value { - Value::Mapping(_) => { - let label = parse_label(value); - let status = parse_status(value)?; - - let mut step = Step::new(name, label); - step.set_status(status); - - for depname in parse_depends(value)?.iter() { - step.add_dependency(depname); - } - - Ok(step) - } - _ => Err(RoadmapError::StepNotMapping), - } -} - -// Get a sequence of depenencies. -fn parse_depends(map: &Value) -> RoadmapResult> { - let key_name = "depends"; - let key = Value::String(key_name.to_string()); - let mut depends: Vec<&str> = vec![]; - - match map.get(&key) { - None => (), - Some(Value::Sequence(deps)) => { - for depname in deps.iter() { - match depname { - Value::String(depname) => depends.push(depname), - _ => return Err(RoadmapError::DependsNotNames), - } - } - } - _ => return Err(RoadmapError::DependsNotNames), - } - - Ok(depends) -} - -// Get label string from a Mapping element, or empty string. -fn parse_label(map: &Value) -> &str { - parse_string("label", map) -} - -// Get status string from a Mapping element. Default to unknown status. -fn parse_status(map: &Value) -> RoadmapResult { - let text = parse_string("status", map); - match Status::from_text(text) { - Some(status) => Ok(status), - _ => Err(RoadmapError::UnknownStatus(text.into())), - } -} - -// Get string value from a Mapping field, or empty string if field -// isn't there. -fn parse_string<'a>(key_name: &str, map: &'a Value) -> &'a str { - let key = Value::String(key_name.to_string()); - match map.get(&key) { - Some(Value::String(s)) => s, - _ => "", - } -} - #[cfg(test)] mod tests { use super::from_yaml; #[test] fn yaml_is_empty() { - if from_yaml("").is_ok() { - panic!("expected a parse error"); - } + assert!(from_yaml("").is_err()); } #[test] fn yaml_is_list() { - if from_yaml("[]").is_ok() { - panic!("expected a parse error"); - } - } - - #[test] - fn yaml_map_entries_not_maps() { - if from_yaml("foo: []").is_ok() { - panic!("expected a parse error"); - } + assert!(from_yaml("[]").is_err()); } #[test] fn yaml_unknown_dep() { - if from_yaml("foo: {depends: [bar]}").is_ok() { - panic!("expected a parse error"); - } + assert!(from_yaml("foo: {depends: [bar]}").is_err()); } #[test] fn yaml_unknown_status() { - if from_yaml(r#"foo: {status: "bar"}"#).is_ok() { - panic!("expected a parse error"); - } + assert!(from_yaml(r#"foo: {status: "bar"}"#).is_err()); } #[test] diff --git a/src/status.rs b/src/status.rs index 9075377..811d1ca 100644 --- a/src/status.rs +++ b/src/status.rs @@ -1,3 +1,5 @@ +use serde::Deserialize; + /// Represent the status of a step in a roadmap. /// /// The unknown status allows the user to not specify the status, and @@ -5,7 +7,8 @@ /// example, a step is inferred to be blocked if any of it /// dependencies are not finished. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize)] +#[serde(rename_all = "lowercase")] pub enum Status { Unknown, Goal, @@ -15,6 +18,12 @@ pub enum Status { Blocked, } +impl Default for Status { + fn default() -> Self { + Self::Unknown + } +} + impl Status { pub fn from_text(text: &str) -> Option { match text { diff --git a/src/step.rs b/src/step.rs index 92b8eec..c37994a 100644 --- a/src/step.rs +++ b/src/step.rs @@ -1,4 +1,5 @@ use super::Status; +use serde::Deserialize; use std::fmt; /// A roadmap step. @@ -6,11 +7,16 @@ use std::fmt; /// See the crate documentation for an example. You /// probably don't want to create steps manually, but via the roadmap /// YAML parsing function. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, Deserialize)] +#[serde(deny_unknown_fields)] pub struct Step { + #[serde(skip)] name: String, - status: Status, + #[serde(default)] label: String, + #[serde(default)] + status: Status, + #[serde(default)] depends: Vec, } @@ -25,6 +31,11 @@ impl Step { } } + /// Set the name of a step. + pub fn set_name(&mut self, name: &str) { + self.name = name.to_string(); + } + /// Return the name of a step. pub fn name(&self) -> &str { &self.name -- cgit v1.2.1