Further improve error messages

chmp · Sep 1, 2024 · f525905 · f525905
1 parent b94935c
commit f525905
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 88 deletions.
diff --git a/serde_arrow/src/internal/deserialization/string_deserializer.rs b/serde_arrow/src/internal/deserialization/string_deserializer.rs
@@ -1,6 +1,6 @@
 use crate::internal::{
     arrow::BytesArrayView,
-    error::{error, fail, Result},
+    error::{fail, Result},
     utils::{Mut, Offset},
 };
 
@@ -39,9 +39,10 @@ impl<'a, O: Offset> StringDeserializer<'a, O> {
     }
 
     pub fn next_required(&mut self) -> Result<&'a str> {
-        self.next()?.ok_or_else(|| {
-            error!("Tried to deserialize a value from StringDeserializer, but value is missing")
-        })
+        let Some(next) = self.next()? else {
+            fail!("Tried to deserialize a value from StringDeserializer, but value is missing")
+        };
+        Ok(next)
     }
 
     pub fn peek_next(&self) -> Result<bool> {

diff --git a/serde_arrow/src/internal/deserialization/utils.rs b/serde_arrow/src/internal/deserialization/utils.rs
@@ -1,6 +1,6 @@
 use crate::internal::{
     arrow::BitsWithOffset,
-    error::{error, fail, Result},
+    error::{fail, Result},
     utils::Offset,
 };
 
@@ -44,7 +44,10 @@ impl<'a, T: Copy> ArrayBufferIterator<'a, T> {
     }
 
     pub fn next_required(&mut self) -> Result<T> {
-        self.next()?.ok_or_else(|| error!("missing value"))
+        let Some(next) = self.next()? else {
+            fail!("missing value");
+        };
+        Ok(next)
     }
 
     pub fn peek_next(&self) -> Result<bool> {

diff --git a/serde_arrow/src/internal/deserializer.rs b/serde_arrow/src/internal/deserializer.rs
@@ -81,19 +81,19 @@ impl<'de> serde::de::Deserializer<'de> for Deserializer<'de> {
     }
 
     fn deserialize_bool<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single bools")
+        fail!("Cannot deserialize single bools")
     }
 
     fn deserialize_byte_buf<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize byte buffers")
+        fail!("Cannot deserialize byte buffers")
     }
 
     fn deserialize_bytes<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize byte arrays")
+        fail!("Cannot deserialize byte arrays")
     }
 
     fn deserialize_char<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single chars")
+        fail!("Cannot deserialize single chars")
     }
 
     fn deserialize_enum<V: Visitor<'de>>(
@@ -102,55 +102,55 @@ impl<'de> serde::de::Deserializer<'de> for Deserializer<'de> {
         _: &'static [&'static str],
         _: V,
     ) -> Result<V::Value> {
-        fail!("cannot deserialize single enums")
+        fail!("Cannot deserialize single enums")
     }
 
     fn deserialize_f32<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single floats")
+        fail!("Cannot deserialize single floats")
     }
 
     fn deserialize_f64<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single floats")
+        fail!("Cannot deserialize single floats")
     }
 
     fn deserialize_i128<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_i16<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_i32<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_i64<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_i8<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_identifier<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single identifiers")
+        fail!("Cannot deserialize single identifiers")
     }
 
     fn deserialize_map<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single maps")
+        fail!("Cannot deserialize single maps")
     }
 
     fn deserialize_option<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single options")
+        fail!("Cannot deserialize single options")
     }
 
     fn deserialize_str<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single strings")
+        fail!("Cannot deserialize single strings")
     }
 
     fn deserialize_string<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single strings")
+        fail!("Cannot deserialize single strings")
     }
 
     fn deserialize_struct<V: Visitor<'de>>(
@@ -159,35 +159,35 @@ impl<'de> serde::de::Deserializer<'de> for Deserializer<'de> {
         _: &'static [&'static str],
         _: V,
     ) -> Result<V::Value> {
-        fail!("cannot deserialize single structs")
+        fail!("Cannot deserialize single structs")
     }
 
     fn deserialize_u128<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_u16<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_u32<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_u64<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_u8<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single integers")
+        fail!("Cannot deserialize single integers")
     }
 
     fn deserialize_unit<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single units")
+        fail!("Cannot deserialize single units")
     }
 
     fn deserialize_unit_struct<V: Visitor<'de>>(self, _: &'static str, _: V) -> Result<V::Value> {
-        fail!("cannot deserialize single units")
+        fail!("Cannot deserialize single units")
     }
 
     fn is_human_readable(&self) -> bool {

diff --git a/serde_arrow/src/internal/error.rs b/serde_arrow/src/internal/error.rs
@@ -211,25 +211,17 @@ impl serde::de::Error for Error {
     }
 }
 
-macro_rules! error {
-    ($($tt:tt)*) => {
-        $crate::internal::error::Error::custom(format!($($tt)*))
-    };
-}
-
-pub(crate) use error;
-
 macro_rules! fail {
     (in $context:expr, $($tt:tt)*) => {
         {
             #[allow(unused)]
             use $crate::internal::error::Context;
             let annotations = $context.annotations();
-            return Err($crate::internal::error::error!($($tt)*).with_annotations(annotations))
+            return Err($crate::internal::error::Error::custom(format!($($tt)*)).with_annotations(annotations))
         }
     };
     ($($tt:tt)*) => {
-        return Err($crate::internal::error::error!($($tt)*))
+        return Err($crate::internal::error::Error::custom(format!($($tt)*)))
     };
 }
 
@@ -305,7 +297,7 @@ impl<E: std::fmt::Display> From<E> for PanicOnErrorError {
 #[test]
 fn error_can_be_converted_to_anyhow() {
     fn func() -> anyhow::Result<()> {
-        Err(error!("dummy"))?;
+        Err(Error::custom("dummy".to_string()))?;
         Ok(())
     }
     assert!(func().is_err());

diff --git a/serde_arrow/src/internal/testing.rs b/serde_arrow/src/internal/testing.rs
@@ -1,7 +1,7 @@
 //! Support for tests
 pub fn assert_error_contains<T, E: std::fmt::Display>(actual: &Result<T, E>, expected: &str) {
     let Err(actual) = actual else {
-        panic!("expected an error, but no error was raised");
+        panic!("Expected an error, but no error was raised");
     };
 
     let actual = actual.to_string();

diff --git a/serde_arrow/src/internal/utils/decimal.rs b/serde_arrow/src/internal/utils/decimal.rs
@@ -198,17 +198,17 @@ fn find_period(s: &[u8]) -> (usize, usize) {
 fn check_all_ascii_zero(s: &[u8], leading: bool) -> Result<()> {
     if s.iter().any(|c| *c != b'0') {
         if leading {
-            fail!("invalid decimal: not enough precision");
+            fail!("Invalid decimal: not enough precision");
         } else {
-            fail!("invalid decimal: not enough scale, the given number would be truncated");
+            fail!("Invalid decimal: not enough scale, the given number would be truncated");
         }
     }
     Ok(())
 }
 
 fn check_all_ascii_digit(s: &[u8]) -> Result<()> {
     if s.iter().any(|c| *c < b'0' || *c > b'9') {
-        fail!("invalid decimal");
+        fail!("Invalid decimal: only ascii digits are supported");
     }
     Ok(())
 }

diff --git a/serde_arrow/src/internal/utils/dsl.rs b/serde_arrow/src/internal/utils/dsl.rs
@@ -18,31 +18,31 @@ impl Term {
     pub fn as_ident(&self) -> Result<&str> {
         match self.as_parts() {
             (name, false, []) => Ok(name),
-            (_, true, _) => fail!("expected identifier, found quoted string"),
-            (_, _, [_, ..]) => fail!("expected identifier, found call"),
+            (_, true, _) => fail!("Expected identifier, found quoted string"),
+            (_, _, [_, ..]) => fail!("Expected identifier, found call"),
         }
     }
 
     pub fn as_string(&self) -> Result<&str> {
         match self.as_parts() {
             (name, true, []) => Ok(name),
-            (_, false, _) => fail!("expected string, found identifier"),
-            (_, _, [_, ..]) => fail!("expected identifier, found call"),
+            (_, false, _) => fail!("Expected string, found identifier"),
+            (_, _, [_, ..]) => fail!("Expected identifier, found call"),
         }
     }
 
     pub fn as_option(&self) -> Result<Option<&Term>> {
         match self.as_parts() {
             ("None", false, []) => Ok(None),
             ("Some", false, [arg]) => Ok(Some(arg)),
-            _ => fail!("expected Some(arg) or None found quoted string"),
+            _ => fail!("Expected Some(arg) or None found quoted string"),
         }
     }
 
     pub fn as_call(&self) -> Result<(&str, &[Term])> {
         match self.as_parts() {
             (name, false, args) => Ok((name, args)),
-            (_, true, _) => fail!("expected call, found quoted string"),
+            (_, true, _) => fail!("Expected call, found quoted string"),
         }
     }
 }
@@ -166,7 +166,7 @@ fn parse_ident_term_name(s: &str) -> Result<(String, &str)> {
     let rest = &s[pos..];
 
     if ident.is_empty() {
-        fail!("no identifier found");
+        fail!("No identifier found");
     }
 
     Ok((ident, rest))
@@ -196,7 +196,7 @@ fn parse_arguments(s: &str) -> Result<(Vec<Term>, &str)> {
 
     let s = s.trim_start();
     let Some(s) = s.strip_prefix(')') else {
-        fail!("mising ')'");
+        fail!("Missing ')'");
     };
 
     Ok((arguments, s))