Merge pull request #570 from EnergySystemsModellingLab/fix-parse-year-str

`parse_year_str`: Handle out-of-order and duplicate years

Author: alexdewar

src/input.rs

@@ -138,6 +138,15 @@ where
     Ok(())
 }
 
+/// Check whether an iterator contains values that are sorted and unique
+pub fn is_sorted_and_unique<T, I>(iter: I) -> bool
+where
+    T: PartialOrd + Clone,
+    I: IntoIterator<Item = T>,
+{
+    iter.into_iter().tuple_windows().all(|(a, b)| a < b)
+}
+
 /// Inserts a key-value pair into a HashMap if the key does not already exist.
 ///
 /// If the key already exists, it returns an error with a message indicating the key's existence.
@@ -200,9 +209,9 @@ pub fn load_model<P: AsRef<Path>>(model_dir: P) -> Result<(Model, AssetPool)> {
 
 #[cfg(test)]
 mod tests {
-    use crate::id::GenericID;
-
     use super::*;
+    use crate::id::GenericID;
+    use rstest::rstest;
     use serde::de::value::{Error as ValueError, F64Deserializer};
     use serde::de::IntoDeserializer;
     use serde::Deserialize;
@@ -324,4 +333,16 @@ mod tests {
         assert!(check_fractions_sum_to_one([f64::INFINITY].into_iter()).is_err());
         assert!(check_fractions_sum_to_one([f64::NAN].into_iter()).is_err());
     }
+
+    #[rstest]
+    #[case(&[], true)]
+    #[case(&[1], true)]
+    #[case(&[1,2], true)]
+    #[case(&[1,2,3,4], true)]
+    #[case(&[2,1],false)]
+    #[case(&[1,1],false)]
+    #[case(&[1,3,2,4], false)]
+    fn test_is_sorted_and_unique(#[case] values: &[u32], #[case] expected: bool) {
+        assert_eq!(is_sorted_and_unique(values), expected)
+    }
 }

src/input/process/availability.rs

@@ -115,15 +115,15 @@ where
             .with_context(|| format!("Process {id} not found"))?;
 
         // Get regions
-        let process_regions = process.regions.clone();
+        let process_regions = &process.regions;
         let record_regions =
-            parse_region_str(&record.regions, &process_regions).with_context(|| {
+            parse_region_str(&record.regions, process_regions).with_context(|| {
                 format!("Invalid region for process {id}. Valid regions are {process_regions:?}")
             })?;
 
         // Get years
-        let process_years = process.years.clone();
-        let record_years = parse_year_str(&record.years, &process_years).with_context(|| {
+        let process_years = &process.years;
+        let record_years = parse_year_str(&record.years, process_years).with_context(|| {
             format!("Invalid year for process {id}. Valid years are {process_years:?}")
         })?;
 

src/input/process/flow.rs

@@ -88,15 +88,15 @@ where
             .with_context(|| format!("Process {id} not found"))?;
 
         // Get regions
-        let process_regions = process.regions.clone();
+        let process_regions = &process.regions;
         let record_regions =
-            parse_region_str(&record.regions, &process_regions).with_context(|| {
+            parse_region_str(&record.regions, process_regions).with_context(|| {
                 format!("Invalid region for process {id}. Valid regions are {process_regions:?}")
             })?;
 
         // Get years
-        let process_years = process.years.clone();
-        let record_years = parse_year_str(&record.years, &process_years).with_context(|| {
+        let process_years = &process.years;
+        let record_years = parse_year_str(&record.years, process_years).with_context(|| {
             format!("Invalid year for process {id}. Valid years are {process_years:?}")
         })?;
 

src/model.rs

@@ -1,7 +1,7 @@
 //! The model represents the static input data provided by the user.
 use crate::agent::AgentMap;
 use crate::commodity::CommodityMap;
-use crate::input::{input_err_msg, read_toml};
+use crate::input::{input_err_msg, is_sorted_and_unique, read_toml};
 use crate::process::ProcessMap;
 use crate::region::{RegionID, RegionMap};
 use crate::time_slice::TimeSliceInfo;
@@ -54,10 +54,7 @@ fn check_milestone_years(years: &[u32]) -> Result<()> {
     ensure!(!years.is_empty(), "`milestone_years` is empty");
 
     ensure!(
-        years[..years.len() - 1]
-            .iter()
-            .zip(years[1..].iter())
-            .all(|(y1, y2)| y1 < y2),
+        is_sorted_and_unique(years),
         "`milestone_years` must be composed of unique values in order"
     );
 

src/year.rs

@@ -1,26 +1,96 @@
 //! Code for working with years.
-use anyhow::{ensure, Result};
+use crate::input::is_sorted_and_unique;
+use anyhow::{ensure, Context, Result};
+use itertools::Itertools;
+
+/// Parse a single year from a string and check it is in `valid_years`
+fn parse_and_validate_year(s: &str, valid_years: &[u32]) -> Option<u32> {
+    let year = s.trim().parse::<u32>().ok()?;
+    if valid_years.binary_search(&year).is_ok() {
+        Some(year)
+    } else {
+        None
+    }
+}
 
 /// Parse a string of years separated by semicolons into a vector of u32 years.
 ///
 /// The string can be either "all" (case-insensitive), a single year, or a semicolon-separated list
 /// of years (e.g. "2020;2021;2022" or "2020; 2021; 2022")
-pub fn parse_year_str(s: &str, milestone_years: &[u32]) -> Result<Vec<u32>> {
+///
+/// # Arguments
+///
+/// - `s` - Input string to parse
+/// - `valid_years` - The possible years which can be referenced in `s` (must be sorted and unique)
+///
+/// # Returns
+///
+/// A [`Vec`] of years or an error.
+///
+/// # Panics
+///
+/// If `valid_years` is unsorted or non-unique.
+pub fn parse_year_str(s: &str, valid_years: &[u32]) -> Result<Vec<u32>> {
+    // We depend on this in `parse_and_validate_year`
+    assert!(
+        is_sorted_and_unique(valid_years),
+        "`valid_years` must be sorted and unique"
+    );
+
     let s = s.trim();
     ensure!(!s.is_empty(), "No years provided");
 
     if s.eq_ignore_ascii_case("all") {
-        return Ok(Vec::from_iter(milestone_years.iter().copied()));
+        return Ok(Vec::from_iter(valid_years.iter().copied()));
     }
 
-    s.split(";")
+    let years: Vec<_> = s
+        .split(";")
         .map(|y| {
-            let year = y.trim().parse::<u32>()?;
-            ensure!(
-                milestone_years.binary_search(&year).is_ok(),
-                "Invalid year {year}"
-            );
-            Ok(year)
+            parse_and_validate_year(y, valid_years).with_context(|| format!("Invalid year: {y}"))
         })
-        .collect()
+        .try_collect()?;
+
+    ensure!(
+        is_sorted_and_unique(&years),
+        "Years must be in order and unique"
+    );
+
+    Ok(years)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::fixture::assert_error;
+    use rstest::rstest;
+
+    #[rstest]
+    #[case("2020", &[2020, 2021], &[2020])]
+    #[case("all", &[2020, 2021], &[2020,2021])]
+    #[case("ALL", &[2020, 2021], &[2020,2021])]
+    #[case(" ALL ", &[2020, 2021], &[2020,2021])]
+    #[case("2020;2021", &[2020, 2021], &[2020,2021])]
+    #[case("  2020;  2021", &[2020, 2021], &[2020,2021])] // whitespace should be stripped
+    fn test_parse_year_str_valid(
+        #[case] input: &str,
+        #[case] milestone_years: &[u32],
+        #[case] expected: &[u32],
+    ) {
+        assert_eq!(parse_year_str(input, milestone_years).unwrap(), expected);
+    }
+
+    #[rstest]
+    #[case("", &[2020], "No years provided")]
+    #[case("2021", &[2020], "Invalid year: 2021")]
+    #[case("a;2020", &[2020], "Invalid year: a")]
+    #[case("2021;2020", &[2020, 2021],"Years must be in order and unique")] // out of order
+    #[case("2021;2020;2021", &[2020, 2021],"Years must be in order and unique")] // duplicate
+    fn test_parse_year_str_invalid(
+        #[case] input: &str,
+        #[case] milestone_years: &[u32],
+        #[case] error_msg: &str,
+    ) {
+        assert_error!(parse_year_str(input, milestone_years), error_msg);
+    }
 }