Error improvements (#138)

* making val_line_error a function

* simplify context

* simplify location

* better error creation

* cleanup

* update context! macro

* more cleanup

* move error context into enum (#139)

* move error context into enum

* tweak

* simplify error rendering

* use static strings where possible

* tweak error kind rendering

* improve coverage

* improve coverage

* cleanup

* remove unused code

* correct imports

* cleanup location logic

Author: samuelcolvin

.pre-commit-config.yaml

@@ -22,13 +22,3 @@ repos:
     entry: make lint-rust
     types: [rust]
     language: system
-  - id: pyright
-    name: Pyright
-    entry: make pyright
-    types: [python]
-    language: system
-  - id: build
-    name: Build
-    entry: make build-dev
-    types: [rust]
-    language: system

Makefile

@@ -105,7 +105,7 @@ testcov-windows: build-cov-windows test
 
 
 .PHONY: all
-all: format build-dev lint pyright test
+all: format build-dev lint test
 
 .PHONY: flame
 flame:

src/build_tools.rs

@@ -6,6 +6,8 @@ use pyo3::prelude::*;
 use pyo3::types::PyDict;
 use pyo3::{FromPyObject, PyErrArguments};
 
+use crate::errors::{pretty_line_errors, ValError};
+
 pub trait SchemaDict<'py> {
     fn get_as<T>(&'py self, key: &str) -> PyResult<Option<T>>
     where
@@ -161,5 +163,4 @@ macro_rules! py_error {
         Err(<$error_type>::new_err(format!($msg, $( $msg_args ),+)))
     };
 }
-use crate::errors::{pretty_line_errors, ValError};
 pub(crate) use py_error;

src/errors/kinds.rs

@@ -1,9 +1,6 @@
-use std::fmt;
-
 use pyo3::prelude::*;
-use pyo3::types::PyDict;
 
-use crate::input::JsonInput;
+use crate::input::{Input, JsonInput};
 
 use super::kinds::ErrorKind;
 use super::location::{LocItem, Location};
@@ -29,6 +26,16 @@ impl<'a> From<Vec<ValLineError<'a>>> for ValError<'a> {
     }
 }
 
+impl<'a> ValError<'a> {
+    pub fn new(kind: ErrorKind, input: &'a impl Input<'a>) -> ValError<'a> {
+        Self::LineErrors(vec![ValLineError::new(kind, input)])
+    }
+
+    pub fn new_with_loc(kind: ErrorKind, input: &'a impl Input<'a>, loc: impl Into<LocItem>) -> ValError<'a> {
+        Self::LineErrors(vec![ValLineError::new_with_loc(kind, input, loc)])
+    }
+}
+
 // ValError used to implement Error, see #78 for removed code
 
 // TODO, remove and replace with just .into()
@@ -44,20 +51,35 @@ pub fn pretty_line_errors(py: Python, line_errors: Vec<ValLineError>) -> String
 /// A `ValLineError` is a single error that occurred during validation which is converted to a `PyLineError`
 /// to eventually form a `ValidationError`.
 /// I don't like the name `ValLineError`, but it's the best I could come up with (for now).
-#[derive(Debug, Default)]
+#[derive(Debug)]
 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>,
-    pub context: Context,
 }
 
 impl<'a> ValLineError<'a> {
+    pub fn new(kind: ErrorKind, input: &'a impl Input<'a>) -> ValLineError<'a> {
+        Self {
+            kind,
+            input_value: input.as_error_value(),
+            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(),
+            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 {
-        self.reverse_location.push(loc_item);
+        self.location.with_outer(loc_item);
         self
     }
 
@@ -70,28 +92,20 @@ 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(),
-            context: self.context,
         }
     }
 }
 
 #[derive(Debug)]
 pub enum InputValue<'a> {
-    None,
     PyAny(&'a PyAny),
     JsonInput(&'a JsonInput),
     String(&'a str),
     PyObject(PyObject),
 }
 
-impl Default for InputValue<'_> {
-    fn default() -> Self {
-        Self::None
-    }
-}
-
 impl<'a> From<PyObject> for InputValue<'a> {
     fn from(py_object: PyObject) -> Self {
         Self::PyObject(py_object)
@@ -101,112 +115,10 @@ impl<'a> From<PyObject> for InputValue<'a> {
 impl<'a> ToPyObject for InputValue<'a> {
     fn to_object(&self, py: Python) -> PyObject {
         match self {
-            Self::None => py.None(),
             Self::PyAny(input) => input.into_py(py),
             Self::JsonInput(input) => input.to_object(py),
             Self::String(input) => input.into_py(py),
             Self::PyObject(py_obj) => py_obj.into_py(py),
         }
     }
 }
-
-#[derive(Debug, Clone, Default)]
-pub struct Context(Vec<(String, ContextValue)>);
-
-impl Context {
-    pub fn new<I: IntoIterator<Item = (String, ContextValue)>>(raw: I) -> Self {
-        Self(raw.into_iter().collect())
-    }
-
-    pub fn is_empty(&self) -> bool {
-        self.0.is_empty()
-    }
-
-    pub fn render(&self, template: String) -> String {
-        let mut rendered = template;
-        for (key, value) in &self.0 {
-            rendered = rendered.replace(&format!("{{{}}}", key), &value.to_string());
-        }
-        rendered
-    }
-}
-
-impl fmt::Display for Context {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let loc = self
-            .0
-            .iter()
-            .map(|(k, v)| format!("{}: {}", k, v))
-            .collect::<Vec<String>>()
-            .join(", ");
-        write!(f, "{{{}}}", loc)
-    }
-}
-
-// maybe this is overkill and we should just use fmt::Display an convert to string when creating Context?
-#[derive(Debug, Clone)]
-pub enum ContextValue {
-    S(String),
-    I(i64),
-    F(f64),
-}
-
-impl fmt::Display for ContextValue {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            ContextValue::S(v) => write!(f, "{}", v),
-            ContextValue::I(v) => write!(f, "{}", v),
-            ContextValue::F(v) => write!(f, "{}", v),
-        }
-    }
-}
-
-impl From<String> for ContextValue {
-    fn from(str: String) -> Self {
-        Self::S(str)
-    }
-}
-
-impl From<&str> for ContextValue {
-    fn from(str: &str) -> Self {
-        Self::S(str.to_string())
-    }
-}
-
-impl From<i64> for ContextValue {
-    fn from(int: i64) -> Self {
-        Self::I(int)
-    }
-}
-
-impl From<usize> for ContextValue {
-    fn from(u: usize) -> Self {
-        Self::I(u as i64)
-    }
-}
-
-impl From<f64> for ContextValue {
-    fn from(f: f64) -> Self {
-        Self::F(f)
-    }
-}
-
-impl ToPyObject for ContextValue {
-    fn to_object(&self, py: Python) -> PyObject {
-        match self {
-            ContextValue::S(v) => v.into_py(py),
-            ContextValue::I(v) => v.into_py(py),
-            ContextValue::F(v) => v.into_py(py),
-        }
-    }
-}
-
-impl ToPyObject for Context {
-    fn to_object(&self, py: Python) -> PyObject {
-        let dict = PyDict::new(py);
-        for (key, value) in &self.0 {
-            dict.set_item(key, value).unwrap();
-        }
-        dict.into_py(py)
-    }
-}

src/errors/line_error.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,4 +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 = 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),
+        }
+    }
+}
+
+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(()),
+        }
+    }
+}
+
+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/location.rs

@@ -2,46 +2,14 @@ use pyo3::prelude::*;
 
 mod kinds;
 mod line_error;
-pub mod location;
+mod location;
 mod validation_exception;
 
 pub use self::kinds::ErrorKind;
-pub use self::line_error::{as_internal, pretty_line_errors, Context, InputValue, ValError, ValLineError, ValResult};
+pub use self::line_error::{as_internal, pretty_line_errors, InputValue, ValError, ValLineError, ValResult};
+pub use self::location::LocItem;
 pub use self::validation_exception::ValidationError;
 
-/// Utility for concisely creating a `ValLineError`
-/// can either take just `py` and a `value` (the given value) in which case kind `ErrorKind::ValueError` is used as kind
-/// e.g. `val_line_error!(py, "the value provided")`
-/// or, `py`, `value` and a mapping of other attributes for `ValLineError`
-/// e.g. `val_line_error!(py, "the value provided", kind=ErrorKind::ExtraForbidden, message="the message")`
-macro_rules! val_line_error {
-    ($($key:ident = $val:expr),+ $(,)?) => {
-        crate::errors::ValLineError {
-            $(
-                $key: $val,
-            )+
-            ..Default::default()
-        }
-    };
-}
-pub(crate) use val_line_error;
-
-/// Utility for concisely creating a `Err(ValError::LineErrors([?]))` containing a single `ValLineError`
-/// Usage matches `val_line_error`
-macro_rules! err_val_error {
-    ($($key:ident = $val:expr),+ $(,)?) => {
-        Err(crate::errors::ValError::LineErrors(vec![crate::errors::val_line_error!($($key = $val),+)]))
-    };
-}
-pub(crate) use err_val_error;
-
-macro_rules! context {
-    ($($k:expr => $v:expr),* $(,)?) => {{
-        crate::errors::Context::new([$(($k.into(), $v.into()),)*])
-    }};
-}
-pub(crate) use context;
-
 pub fn py_err_string(py: Python, err: PyErr) -> String {
     let value = err.value(py);
     match value.get_type().name() {