Repository: dimastbk/python-calamine (language: python)

Issue:
Make exceptions more detailed
> I think more of a design decision for Calamine but are you expecting to implement more detailed error types? If so would probably be better to use those instead of a generic error like this

_Originally posted by @WillAyd in https://github.com/pandas-dev/pandas/pull/54998#discussion_r1316152239_            


Discussion / hints:
At least:
* ZipError
* XmlError
* VbaError
* PasswordError (?) - now only in xls
* Io (?)

Fix patch:
diff --git a/python/python_calamine/__init__.py b/python/python_calamine/__init__.py
index b192511..9771394 100644
--- a/python/python_calamine/__init__.py
+++ b/python/python_calamine/__init__.py
@@ -2,9 +2,13 @@
     CalamineError,
     CalamineSheet,
     CalamineWorkbook,
+    PasswordError,
     SheetMetadata,
     SheetTypeEnum,
     SheetVisibleEnum,
+    WorksheetNotFound,
+    XmlError,
+    ZipError,
     load_workbook,
 )
 
@@ -12,8 +16,12 @@
     "CalamineError",
     "CalamineSheet",
     "CalamineWorkbook",
+    "PasswordError",
     "SheetMetadata",
     "SheetTypeEnum",
     "SheetVisibleEnum",
+    "WorksheetNotFound",
+    "XmlError",
+    "ZipError",
     "load_workbook",
 )
diff --git a/python/python_calamine/_python_calamine.pyi b/python/python_calamine/_python_calamine.pyi
index 093d7a6..67037a4 100644
--- a/python/python_calamine/_python_calamine.pyi
+++ b/python/python_calamine/_python_calamine.pyi
@@ -109,6 +109,10 @@ class CalamineWorkbook:
     def get_sheet_by_index(self, index: int) -> CalamineSheet: ...
 
 class CalamineError(Exception): ...
+class PasswordError(CalamineError): ...
+class WorksheetNotFound(CalamineError): ...
+class XmlError(CalamineError): ...
+class ZipError(CalamineError): ...
 
 def load_workbook(
     path_or_filelike: str | os.PathLike | ReadBuffer,
diff --git a/src/lib.rs b/src/lib.rs
index 8ccceab..9e83bcf 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -3,8 +3,8 @@ use pyo3::prelude::*;
 mod types;
 mod utils;
 use crate::types::{
-    CalamineError, CalamineSheet, CalamineWorkbook, CellValue, SheetMetadata, SheetTypeEnum,
-    SheetVisibleEnum,
+    CalamineError, CalamineSheet, CalamineWorkbook, CellValue, PasswordError, SheetMetadata,
+    SheetTypeEnum, SheetVisibleEnum, WorksheetNotFound, XmlError, ZipError,
 };
 
 #[pyfunction]
@@ -21,5 +21,9 @@ fn _python_calamine(py: Python, m: &PyModule) -> PyResult<()> {
     m.add_class::<SheetTypeEnum>()?;
     m.add_class::<SheetVisibleEnum>()?;
     m.add("CalamineError", py.get_type::<CalamineError>())?;
+    m.add("PasswordError", py.get_type::<PasswordError>())?;
+    m.add("WorksheetNotFound", py.get_type::<WorksheetNotFound>())?;
+    m.add("XmlError", py.get_type::<XmlError>())?;
+    m.add("ZipError", py.get_type::<ZipError>())?;
     Ok(())
 }
diff --git a/src/types/mod.rs b/src/types/mod.rs
index 1c202d3..dc66572 100644
--- a/src/types/mod.rs
+++ b/src/types/mod.rs
@@ -9,3 +9,7 @@ pub use sheet::{CalamineSheet, SheetMetadata, SheetTypeEnum, SheetVisibleEnum};
 pub use workbook::CalamineWorkbook;
 
 create_exception!(python_calamine, CalamineError, PyException);
+create_exception!(python_calamine, PasswordError, CalamineError);
+create_exception!(python_calamine, WorksheetNotFound, CalamineError);
+create_exception!(python_calamine, XmlError, CalamineError);
+create_exception!(python_calamine, ZipError, CalamineError);
diff --git a/src/types/workbook.rs b/src/types/workbook.rs
index 1241029..36c0d1d 100644
--- a/src/types/workbook.rs
+++ b/src/types/workbook.rs
@@ -9,7 +9,7 @@ use pyo3::types::{PyString, PyType};
 use pyo3_file::PyFileLikeObject;
 
 use crate::utils::err_to_py;
-use crate::{CalamineError, CalamineSheet, SheetMetadata};
+use crate::{CalamineSheet, SheetMetadata, WorksheetNotFound};
 
 enum SheetsEnum {
     File(Sheets<BufReader<File>>),
@@ -40,16 +40,6 @@ impl SheetsEnum {
             SheetsEnum::FileLike(f) => f.worksheet_range(name),
         }
     }
-
-    fn worksheet_range_at(
-        &mut self,
-        index: usize,
-    ) -> Option<Result<calamine::Range<calamine::Data>, Error>> {
-        match self {
-            SheetsEnum::File(f) => f.worksheet_range_at(index),
-            SheetsEnum::FileLike(f) => f.worksheet_range_at(index),
-        }
-    }
 }
 
 #[pyclass]
@@ -164,14 +154,8 @@ impl CalamineWorkbook {
         let name = self
             .sheet_names
             .get(index)
-            .ok_or_else(|| CalamineError::new_err("Workbook is empty"))?
+            .ok_or_else(|| WorksheetNotFound::new_err(format!("Worksheet '{}' not found", index)))?
             .to_string();
-        let range = self
-            .sheets
-            .worksheet_range_at(index)
-            .unwrap_or_else(|| Err(Error::Msg("Workbook is empty")))
-            .map_err(err_to_py)?;
-
-        Ok(CalamineSheet::new(name, range))
+        self.get_sheet_by_name(&name)
     }
 }
diff --git a/src/utils.rs b/src/utils.rs
index 5c00b1f..f79bd0a 100644
--- a/src/utils.rs
+++ b/src/utils.rs
@@ -1,12 +1,46 @@
-use calamine::Error;
+use calamine::{Error, OdsError, XlsError, XlsbError, XlsxError};
 use pyo3::exceptions::PyIOError;
 use pyo3::PyErr;
 
-use crate::types::CalamineError;
+use crate::{CalamineError, PasswordError, WorksheetNotFound, XmlError, ZipError};
 
 pub fn err_to_py(e: Error) -> PyErr {
     match e {
         Error::Io(err) => PyIOError::new_err(err.to_string()),
+        Error::Ods(ref err) => match err {
+            OdsError::Io(error) => PyIOError::new_err(error.to_string()),
+            OdsError::Zip(error) => ZipError::new_err(error.to_string()),
+            OdsError::Xml(error) => XmlError::new_err(error.to_string()),
+            OdsError::XmlAttr(error) => XmlError::new_err(error.to_string()),
+            OdsError::Password => PasswordError::new_err(err.to_string()),
+            OdsError::WorksheetNotFound(error) => WorksheetNotFound::new_err(error.to_string()),
+            _ => CalamineError::new_err(err.to_string()),
+        },
+        Error::Xls(ref err) => match err {
+            XlsError::Io(error) => PyIOError::new_err(error.to_string()),
+            XlsError::Password => PasswordError::new_err(err.to_string()),
+            XlsError::WorksheetNotFound(error) => WorksheetNotFound::new_err(error.to_string()),
+            _ => CalamineError::new_err(err.to_string()),
+        },
+        Error::Xlsx(ref err) => match err {
+            XlsxError::Io(error) => PyIOError::new_err(error.to_string()),
+            XlsxError::Zip(error) => ZipError::new_err(error.to_string()),
+            XlsxError::Xml(error) => XmlError::new_err(error.to_string()),
+            XlsxError::XmlAttr(error) => XmlError::new_err(error.to_string()),
+            XlsxError::XmlEof(error) => XmlError::new_err(error.to_string()),
+            XlsxError::Password => PasswordError::new_err(err.to_string()),
+            XlsxError::WorksheetNotFound(error) => WorksheetNotFound::new_err(error.to_string()),
+            _ => CalamineError::new_err(err.to_string()),
+        },
+        Error::Xlsb(ref err) => match err {
+            XlsbError::Io(error) => PyIOError::new_err(error.to_string()),
+            XlsbError::Zip(error) => ZipError::new_err(error.to_string()),
+            XlsbError::Xml(error) => XmlError::new_err(error.to_string()),
+            XlsbError::XmlAttr(error) => XmlError::new_err(error.to_string()),
+            XlsbError::Password => PasswordError::new_err(err.to_string()),
+            XlsbError::WorksheetNotFound(error) => WorksheetNotFound::new_err(error.to_string()),
+            _ => CalamineError::new_err(err.to_string()),
+        },
         _ => CalamineError::new_err(e.to_string()),
     }
 }


Test patch:
diff --git a/tests/data/empty_file.ods b/tests/data/empty_file.ods
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/empty_file.xlsb b/tests/data/empty_file.xlsb
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/empty_file.xlsx b/tests/data/empty_file.xlsx
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/password.ods b/tests/data/password.ods
new file mode 100644
index 0000000..f5517ea
Binary files /dev/null and b/tests/data/password.ods differ
diff --git a/tests/data/password.xls b/tests/data/password.xls
new file mode 100644
index 0000000..88c926d
Binary files /dev/null and b/tests/data/password.xls differ
diff --git a/tests/data/password.xlsb b/tests/data/password.xlsb
new file mode 100644
index 0000000..ff70e78
Binary files /dev/null and b/tests/data/password.xlsb differ
diff --git a/tests/data/password.xlsx b/tests/data/password.xlsx
new file mode 100644
index 0000000..9807136
Binary files /dev/null and b/tests/data/password.xlsx differ
diff --git a/tests/test_base.py b/tests/test_base.py
index 1b1c1ec..0b239ef 100644
--- a/tests/test_base.py
+++ b/tests/test_base.py
@@ -3,7 +3,7 @@
 from pathlib import Path
 
 import pytest
-from python_calamine import CalamineWorkbook
+from python_calamine import CalamineWorkbook, PasswordError, WorksheetNotFound, ZipError
 
 PATH = Path(__file__).parent / "data"
 
@@ -218,6 +218,62 @@ def test_nrows():
     ]
 
 
+@pytest.mark.parametrize(
+    "path",
+    [
+        PATH / "base.xlsx",
+        PATH / "base.xls",
+        PATH / "base.xlsb",
+        PATH / "base.ods",
+    ],
+)
+def test_worksheet_errors(path):
+    reader = CalamineWorkbook.from_object(path)
+    with pytest.raises(WorksheetNotFound):
+        reader.get_sheet_by_name("Sheet4")
+
+
+@pytest.mark.parametrize(
+    "path",
+    [
+        PATH / "password.xlsx",
+        PATH / "password.xls",
+        PATH / "password.xlsb",
+        PATH / "password.ods",
+    ],
+)
+def test_password_errors(path):
+    with pytest.raises(PasswordError):
+        CalamineWorkbook.from_object(path)
+
+
+@pytest.mark.parametrize(
+    "path",
+    [
+        PATH / "empty_file.xlsx",
+        PATH / "empty_file.xlsb",
+        PATH / "empty_file.ods",
+    ],
+)
+def test_zip_errors(path):
+    with pytest.raises(ZipError):
+        CalamineWorkbook.from_path(path)
+
+
+@pytest.mark.parametrize(
+    "path",
+    [
+        PATH / "non_existent_file.xlsx",
+        PATH / "non_existent_file.xls",
+        PATH / "non_existent_file.xlsb",
+        PATH / "non_existent_file.ods",
+    ],
+)
+def test_io_errors(path):
+    with pytest.raises(IOError):
+        CalamineWorkbook.from_path(path)
+
+
 @pytest.mark.parametrize(
     "path",
     [