cleanup location logic

Author: samuelcolvin

src/errors/kinds.rs

@@ -7,7 +7,7 @@ use strum::{Display, EnumMessage};
 /// NOTE: if an error has parameters:
 /// * the variables in the message need to match the enum struct
 /// * you need to add an entry to the `render` enum to render the error message as a template
-/// * you need to ad an entry to the `py_dict` enum to generate `ctx` for error messages
+/// * you need to add an entry to the `py_dict` enum to generate `ctx` for error messages
 #[derive(Debug, Display, EnumMessage, Clone)]
 #[strum(serialize_all = "snake_case")]
 pub enum ErrorKind {

src/errors/line_error.rs

@@ -3,7 +3,7 @@ use pyo3::prelude::*;
 use crate::input::{Input, JsonInput};
 
 use super::kinds::ErrorKind;
-use super::location::{new_location, LocItem, Location};
+use super::location::{LocItem, Location};
 use super::validation_exception::{pretty_py_line_errors, PyLineError};
 
 pub type ValResult<'a, T> = Result<T, ValError<'a>>;
@@ -55,7 +55,7 @@ pub fn pretty_line_errors(py: Python, line_errors: Vec<ValLineError>) -> String
 pub struct ValLineError<'a> {
     pub kind: ErrorKind,
     // location is reversed so that adding an "outer" location item is pushing, it's reversed before showing to the user
-    pub reverse_location: Location,
+    pub location: Location,
     pub input_value: InputValue<'a>,
 }
 
@@ -64,25 +64,22 @@ impl<'a> ValLineError<'a> {
         Self {
             kind,
             input_value: input.as_error_value(),
-            reverse_location: None,
+            location: Location::default(),
         }
     }
 
     pub fn new_with_loc(kind: ErrorKind, input: &'a impl Input<'a>, loc: impl Into<LocItem>) -> ValLineError<'a> {
         Self {
             kind,
             input_value: input.as_error_value(),
-            reverse_location: Some(vec![loc.into()]),
+            location: Location::new_some(loc.into()),
         }
     }
 
     /// location is stored reversed so it's quicker to add "outer" items as that's what we always do
     /// hence `push` here instead of `insert`
     pub fn with_outer_location(mut self, loc_item: LocItem) -> Self {
-        match self.reverse_location {
-            Some(ref mut rev_loc) => rev_loc.push(loc_item),
-            None => self.reverse_location = new_location(loc_item),
-        };
+        self.location.with_outer(loc_item);
         self
     }
 
@@ -95,7 +92,7 @@ impl<'a> ValLineError<'a> {
     pub fn into_new<'b>(self, py: Python) -> ValLineError<'b> {
         ValLineError {
             kind: self.kind,
-            reverse_location: self.reverse_location,
+            location: self.location,
             input_value: self.input_value.to_object(py).into(),
         }
     }

src/errors/location.rs

@@ -1,6 +1,7 @@
 use std::fmt;
 
 use pyo3::prelude::*;
+use pyo3::types::PyList;
 
 /// Used to store individual items of the error location, e.g. a string for key/field names
 /// or a number for array indices.
@@ -51,21 +52,59 @@ impl ToPyObject for LocItem {
 /// Error locations are represented by a vector of `LocItem`s.
 /// e.g. if the error occurred in the third member of a list called `foo`,
 /// the location would be `["foo", 2]`.
-pub type Location = Option<Vec<LocItem>>;
+/// Note: location in List is stored in **REVERSE** so adding an "outer" item to location involves
+/// pushing to the vec which is faster than inserting and shifting everything along.
+/// Then when "using" location in `Display` and `ToPyObject` order has to be reversed
+#[derive(Debug, Clone)]
+pub enum Location {
+    // no location, avoid creating an unnecessary vec
+    Empty,
+    // store the in a vec of LocItems, Note: this is the REVERSE of location, see above
+    // we could perhaps use a smallvec or similar here, probably only worth it if we store a Cow in LocItem
+    List(Vec<LocItem>),
+}
+
+impl Default for Location {
+    fn default() -> Self {
+        Self::Empty
+    }
+}
+
+impl ToPyObject for Location {
+    fn to_object(&self, py: Python<'_>) -> PyObject {
+        match self {
+            Self::List(loc) => loc.iter().rev().collect::<Vec<_>>().to_object(py),
+            Self::Empty => PyList::empty(py).to_object(py),
+        }
+    }
+}
 
-pub fn reverse_location(location: Location) -> Location {
-    match location {
-        Some(loc) => match loc.len() {
-            0..=1 => Some(loc),
-            _ => Some(loc.into_iter().rev().collect()),
-        },
-        None => None,
+impl fmt::Display for Location {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::List(loc) => {
+                let loc_str = loc.iter().rev().map(|i| i.to_string()).collect::<Vec<_>>();
+                writeln!(f, "{}", loc_str.join(" -> "))
+            }
+            Self::Empty => Ok(()),
+        }
     }
 }
 
-pub fn new_location(item: LocItem) -> Location {
-    // 3 is plucked out of the air, should it just be 1?
-    let mut loc = Vec::with_capacity(3);
-    loc.push(item);
-    Some(loc)
+impl Location {
+    /// create a new location vec with a value, 3 is plucked out of thin air, should it just be 1?
+    pub fn new_some(item: LocItem) -> Self {
+        let mut loc = Vec::with_capacity(3);
+        loc.push(item);
+        Self::List(loc)
+    }
+
+    pub fn with_outer(&mut self, loc_item: LocItem) {
+        match self {
+            Self::List(ref mut loc) => loc.push(loc_item),
+            Self::Empty => {
+                *self = Self::new_some(loc_item);
+            }
+        };
+    }
 }

src/errors/validation_exception.rs

@@ -4,9 +4,7 @@ use std::fmt::Write;
 
 use pyo3::exceptions::PyValueError;
 use pyo3::prelude::*;
-use pyo3::types::{PyDict, PyList};
-
-use crate::errors::location::reverse_location;
+use pyo3::types::PyDict;
 
 use crate::input::repr_string;
 
@@ -146,7 +144,7 @@ impl<'a> IntoPy<PyLineError> for ValLineError<'a> {
     fn into_py(self, py: Python<'_>) -> PyLineError {
         PyLineError {
             kind: self.kind,
-            location: reverse_location(self.reverse_location),
+            location: self.location,
             input_value: self.input_value.to_object(py),
         }
     }
@@ -157,7 +155,7 @@ impl<'a> From<PyLineError> for ValLineError<'a> {
     fn from(py_line_error: PyLineError) -> Self {
         Self {
             kind: py_line_error.kind,
-            reverse_location: reverse_location(py_line_error.location),
+            location: py_line_error.location,
             input_value: py_line_error.input_value.into(),
         }
     }
@@ -167,11 +165,7 @@ impl PyLineError {
     pub fn as_dict(&self, py: Python) -> PyResult<PyObject> {
         let dict = PyDict::new(py);
         dict.set_item("kind", self.kind.to_string())?;
-        if let Some(ref location) = self.location {
-            dict.set_item("loc", location.to_object(py))?;
-        } else {
-            dict.set_item("loc", PyList::empty(py).to_object(py))?;
-        }
+        dict.set_item("loc", self.location.to_object(py))?;
         dict.set_item("message", self.kind.render())?;
         dict.set_item("input_value", &self.input_value)?;
         if let Some(context) = self.kind.py_dict(py)? {
@@ -182,14 +176,7 @@ impl PyLineError {
 
     fn pretty(&self, py: Option<Python>) -> Result<String, fmt::Error> {
         let mut output = String::with_capacity(200);
-        if let Some(ref location) = self.location {
-            let loc = location
-                .iter()
-                .map(|i| i.to_string())
-                .collect::<Vec<String>>()
-                .join(" -> ");
-            writeln!(output, "{}", &loc)?;
-        }
+        write!(output, "{}", self.location)?;
 
         write!(output, "  {} [kind={}", self.kind.render(), self.kind)?;