rust-dirstate-status: rust-cpython bindings for `dirstate.status`

The ref-sharing mechanism has improved, but its ergonomics still left a bit
to be desired, as expected.

Differential Revision: https://phab.mercurial-scm.org/D7059

rust/hg-cpython/src/dirstate/status.rs

@@ -0,0 +1,82 b''
+// status.rs
+//
+// Copyright 2019, Raphaël Gomès <[e-mail]>
+//
+// This software may be used and distributed according to the terms of the
+// GNU General Public License version 2 or any later version.
+
+//! Bindings for the `hg::status` module provided by the
+//! `hg-core` crate. From Python, this will be seen as `rustext.dirstate.status`.
+//!
+
+use crate::dirstate::DirstateMap;
+use cpython::exc::ValueError;
+use cpython::{
+    PyBytes, PyErr, PyList, PyObject, PyResult, Python, PythonObject,
+    ToPyObject,
+};
+use hg::utils::files::get_path_from_bytes;
+
+use hg::utils::hg_path::HgPath;
+use hg::{status, utils::hg_path::HgPathBuf};
+
+/// This will be useless once trait impls for collection are added to `PyBytes`
+/// upstream.
+fn collect_pybytes_list<P: AsRef<HgPath>>(
+    py: Python,
+    collection: &[P],
+) -> PyList {
+    let list = PyList::new(py, &[]);
+
+    for (i, path) in collection.iter().enumerate() {
+        list.insert_item(
+            py,
+            i,
+            PyBytes::new(py, path.as_ref().as_bytes()).into_object(),
+        )
+    }
+
+    list
+}
+
+pub fn status_wrapper(
+    py: Python,
+    dmap: DirstateMap,
+    root_dir: PyObject,
+    files: PyList,
+    list_clean: bool,
+    last_normal_time: i64,
+    check_exec: bool,
+) -> PyResult<(PyList, PyList, PyList, PyList, PyList, PyList, PyList)> {
+    let bytes = root_dir.extract::<PyBytes>(py)?;
+    let root_dir = get_path_from_bytes(bytes.data(py));
+
+    let dmap: DirstateMap = dmap.to_py_object(py);
+    let dmap = dmap.get_inner(py);
+
+    let files: PyResult<Vec<HgPathBuf>> = files
+        .iter(py)
+        .map(|f| Ok(HgPathBuf::from_bytes(f.extract::<PyBytes>(py)?.data(py))))
+        .collect();
+    let files = files?;
+
+    let (lookup, status_res) = status(
+        &dmap,
+        &root_dir,
+        &files,
+        list_clean,
+        last_normal_time,
+        check_exec,
+    )
+    .map_err(|e| PyErr::new::<ValueError, _>(py, e.to_string()))?;
+
+    let modified = collect_pybytes_list(py, status_res.modified.as_ref());
+    let added = collect_pybytes_list(py, status_res.added.as_ref());
+    let removed = collect_pybytes_list(py, status_res.removed.as_ref());
+    let deleted = collect_pybytes_list(py, status_res.deleted.as_ref());
+    let clean = collect_pybytes_list(py, status_res.clean.as_ref());
+    let lookup = collect_pybytes_list(py, lookup.as_ref());
+    let unknown = PyList::new(py, &[]);
+
+    Ok((lookup, modified, added, removed, deleted, unknown, clean))
+}

rust/hg-cpython/src/dirstate.rs

@@ -1,114 +1,132 b''
 // dirstate.rs
 //
 // Copyright 2019 Raphaël Gomès <[e-mail]>
 //
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
 //! Bindings for the `hg::dirstate` module provided by the
 //! `hg-core` package.
 //!
 //! From Python, this will be seen as `mercurial.rustext.dirstate`
 mod copymap;
 mod dirs_multiset;
 mod dirstate_map;
-use crate::dirstate::{dirs_multiset::Dirs, dirstate_map::DirstateMap};
+mod status;
+use crate::dirstate::{
+    dirs_multiset::Dirs, dirstate_map::DirstateMap, status::status_wrapper,
+};
 use cpython::{
-    exc, PyBytes, PyDict, PyErr, PyModule, PyObject, PyResult, PySequence,
-    Python,
+    exc, PyBytes, PyDict, PyErr, PyList, PyModule, PyObject, PyResult,
+    PySequence, Python,
 };
 use hg::{
     utils::hg_path::HgPathBuf, DirstateEntry, DirstateParseError, EntryState,
     StateMap,
 };
 use libc::{c_char, c_int};
 use std::convert::TryFrom;
 
 // C code uses a custom `dirstate_tuple` type, checks in multiple instances
 // for this type, and raises a Python `Exception` if the check does not pass.
 // Because this type differs only in name from the regular Python tuple, it
 // would be a good idea in the near future to remove it entirely to allow
 // for a pure Python tuple of the same effective structure to be used,
 // rendering this type and the capsule below useless.
 py_capsule_fn!(
     from mercurial.cext.parsers import make_dirstate_tuple_CAPI
         as make_dirstate_tuple_capi
         signature (
             state: c_char,
             mode: c_int,
             size: c_int,
             mtime: c_int,
         ) -> *mut RawPyObject
 );
 
 pub fn make_dirstate_tuple(
     py: Python,
     entry: &DirstateEntry,
 ) -> PyResult<PyObject> {
     // might be silly to retrieve capsule function in hot loop
     let make = make_dirstate_tuple_capi::retrieve(py)?;
 
     let &DirstateEntry {
         state,
         mode,
         size,
         mtime,
     } = entry;
     // Explicitly go through u8 first, then cast to platform-specific `c_char`
     // because Into<u8> has a specific implementation while `as c_char` would
     // just do a naive enum cast.
     let state_code: u8 = state.into();
 
     let maybe_obj = unsafe {
         let ptr = make(state_code as c_char, mode, size, mtime);
         PyObject::from_owned_ptr_opt(py, ptr)
     };
     maybe_obj.ok_or_else(|| PyErr::fetch(py))
 }
 
 pub fn extract_dirstate(py: Python, dmap: &PyDict) -> Result<StateMap, PyErr> {
     dmap.items(py)
         .iter()
         .map(|(filename, stats)| {
             let stats = stats.extract::<PySequence>(py)?;
             let state = stats.get_item(py, 0)?.extract::<PyBytes>(py)?;
             let state = EntryState::try_from(state.data(py)[0]).map_err(
                 |e: DirstateParseError| {
                     PyErr::new::<exc::ValueError, _>(py, e.to_string())
                 },
             )?;
             let mode = stats.get_item(py, 1)?.extract(py)?;
             let size = stats.get_item(py, 2)?.extract(py)?;
             let mtime = stats.get_item(py, 3)?.extract(py)?;
             let filename = filename.extract::<PyBytes>(py)?;
             let filename = filename.data(py);
             Ok((
                 HgPathBuf::from(filename.to_owned()),
                 DirstateEntry {
                     state,
                     mode,
                     size,
                     mtime,
                 },
             ))
         })
         .collect()
 }
 
 /// Create the module, with `__package__` given from parent
 pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
     let dotted_name = &format!("{}.dirstate", package);
     let m = PyModule::new(py, dotted_name)?;
 
     m.add(py, "__package__", package)?;
     m.add(py, "__doc__", "Dirstate - Rust implementation")?;
 
     m.add_class::<Dirs>(py)?;
     m.add_class::<DirstateMap>(py)?;
+    m.add(
+        py,
+        "status",
+        py_fn!(
+            py,
+            status_wrapper(
+                dmap: DirstateMap,
+                root_dir: PyObject,
+                files: PyList,
+                list_clean: bool,
+                last_normal_time: i64,
+                check_exec: bool
+            )
+        ),
+    )?;
 
     let sys = PyModule::import(py, "sys")?;
     let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?;
     sys_modules.set_item(py, dotted_name, &m)?;
 
     Ok(m)
 }

rust/hg-cpython/src/dirstate/dirstate_map.rs

@@ -1,510 +1,516 b''
 // dirstate_map.rs
 //
 // Copyright 2019 Raphaël Gomès <[e-mail]>
 //
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
 //! Bindings for the `hg::dirstate::dirstate_map` file provided by the
 //! `hg-core` package.
 
-use std::cell::RefCell;
+use std::cell::{Ref, RefCell};
 use std::convert::TryInto;
 use std::time::Duration;
 
 use cpython::{
     exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyObject,
     PyResult, PyTuple, Python, PythonObject, ToPyObject,
 };
 
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
     dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
     ref_sharing::{PyLeakedRef, PySharedRefCell},
 };
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirstateEntry, DirstateMap as RustDirstateMap,
     DirstateParents, DirstateParseError, EntryState, StateMapIter,
     PARENT_SIZE,
 };
 
 // TODO
 //     This object needs to share references to multiple members of its Rust
 //     inner struct, namely `copy_map`, `dirs` and `all_dirs`.
 //     Right now `CopyMap` is done, but it needs to have an explicit reference
 //     to `RustDirstateMap` which itself needs to have an encapsulation for
 //     every method in `CopyMap` (copymapcopy, etc.).
 //     This is ugly and hard to maintain.
 //     The same logic applies to `dirs` and `all_dirs`, however the `Dirs`
 //     `py_class!` is already implemented and does not mention
 //     `RustDirstateMap`, rightfully so.
 //     All attributes also have to have a separate refcount data attribute for
 //     leaks, with all methods that go along for reference sharing.
 py_class!(pub class DirstateMap |py| {
     data inner: PySharedRefCell<RustDirstateMap>;
 
     def __new__(_cls, _root: PyObject) -> PyResult<Self> {
         let inner = RustDirstateMap::default();
         Self::create_instance(
             py,
             PySharedRefCell::new(inner),
         )
     }
 
     def clear(&self) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.clear();
         Ok(py.None())
     }
 
     def get(
         &self,
         key: PyObject,
         default: Option<PyObject> = None
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self.inner(py).borrow().get(HgPath::new(key.data(py))) {
             Some(entry) => {
                 Ok(Some(make_dirstate_tuple(py, entry)?))
             },
             None => Ok(default)
         }
     }
 
     def addfile(
         &self,
         f: PyObject,
         oldstate: PyObject,
         state: PyObject,
         mode: PyObject,
         size: PyObject,
         mtime: PyObject
     ) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.add_file(
             HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
             oldstate.extract::<PyBytes>(py)?.data(py)[0]
                 .try_into()
                 .map_err(|e: DirstateParseError| {
                     PyErr::new::<exc::ValueError, _>(py, e.to_string())
                 })?,
             DirstateEntry {
                 state: state.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
                 mode: mode.extract(py)?,
                 size: size.extract(py)?,
                 mtime: mtime.extract(py)?,
             },
         );
         Ok(py.None())
     }
 
     def removefile(
         &self,
         f: PyObject,
         oldstate: PyObject,
         size: PyObject
     ) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?
             .remove_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
                 oldstate.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
                 size.extract(py)?,
             )
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
                     py,
                     "Dirstate error".to_string(),
                 ))
             })?;
         Ok(py.None())
     }
 
     def dropfile(
         &self,
         f: PyObject,
         oldstate: PyObject
     ) -> PyResult<PyBool> {
         self.inner_shared(py).borrow_mut()?
             .drop_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
                 oldstate.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
             )
             .and_then(|b| Ok(b.to_py_object(py)))
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
                     py,
                     "Dirstate error".to_string(),
                 ))
             })
     }
 
     def clearambiguoustimes(
         &self,
         files: PyObject,
         now: PyObject
     ) -> PyResult<PyObject> {
         let files: PyResult<Vec<HgPathBuf>> = files
             .iter(py)?
             .map(|filename| {
                 Ok(HgPathBuf::from_bytes(
                     filename?.extract::<PyBytes>(py)?.data(py),
                 ))
             })
             .collect();
         self.inner_shared(py).borrow_mut()?
             .clear_ambiguous_times(files?, now.extract(py)?);
         Ok(py.None())
     }
 
     // TODO share the reference
     def nonnormalentries(&self) -> PyResult<PyObject> {
         let (non_normal, other_parent) =
             self.inner(py).borrow().non_normal_other_parent_entries();
 
         let locals = PyDict::new(py);
         locals.set_item(
             py,
             "non_normal",
             non_normal
                 .iter()
                 .map(|v| PyBytes::new(py, v.as_ref()))
                 .collect::<Vec<PyBytes>>()
                 .to_py_object(py),
         )?;
         locals.set_item(
             py,
             "other_parent",
             other_parent
                 .iter()
                 .map(|v| PyBytes::new(py, v.as_ref()))
                 .collect::<Vec<PyBytes>>()
                 .to_py_object(py),
         )?;
 
         py.eval("set(non_normal), set(other_parent)", None, Some(&locals))
     }
 
     def hastrackeddir(&self, d: PyObject) -> PyResult<PyBool> {
         let d = d.extract::<PyBytes>(py)?;
         Ok(self.inner_shared(py).borrow_mut()?
             .has_tracked_dir(HgPath::new(d.data(py)))
             .to_py_object(py))
     }
 
     def hasdir(&self, d: PyObject) -> PyResult<PyBool> {
         let d = d.extract::<PyBytes>(py)?;
         Ok(self.inner_shared(py).borrow_mut()?
             .has_dir(HgPath::new(d.data(py)))
             .to_py_object(py))
     }
 
     def parents(&self, st: PyObject) -> PyResult<PyTuple> {
         self.inner_shared(py).borrow_mut()?
             .parents(st.extract::<PyBytes>(py)?.data(py))
             .and_then(|d| {
                 Ok((PyBytes::new(py, &d.p1), PyBytes::new(py, &d.p2))
                     .to_py_object(py))
             })
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
                     py,
                     "Dirstate error".to_string(),
                 ))
             })
     }
 
     def setparents(&self, p1: PyObject, p2: PyObject) -> PyResult<PyObject> {
         let p1 = extract_node_id(py, &p1)?;
         let p2 = extract_node_id(py, &p2)?;
 
         self.inner_shared(py).borrow_mut()?
             .set_parents(&DirstateParents { p1, p2 });
         Ok(py.None())
     }
 
     def read(&self, st: PyObject) -> PyResult<Option<PyObject>> {
         match self.inner_shared(py).borrow_mut()?
             .read(st.extract::<PyBytes>(py)?.data(py))
         {
             Ok(Some(parents)) => Ok(Some(
                 (PyBytes::new(py, &parents.p1), PyBytes::new(py, &parents.p2))
                     .to_py_object(py)
                     .into_object(),
             )),
             Ok(None) => Ok(Some(py.None())),
             Err(_) => Err(PyErr::new::<exc::OSError, _>(
                 py,
                 "Dirstate error".to_string(),
             )),
         }
     }
     def write(
         &self,
         p1: PyObject,
         p2: PyObject,
         now: PyObject
     ) -> PyResult<PyBytes> {
         let now = Duration::new(now.extract(py)?, 0);
         let parents = DirstateParents {
             p1: extract_node_id(py, &p1)?,
             p2: extract_node_id(py, &p2)?,
         };
 
         match self.inner_shared(py).borrow_mut()?.pack(parents, now) {
             Ok(packed) => Ok(PyBytes::new(py, &packed)),
             Err(_) => Err(PyErr::new::<exc::OSError, _>(
                 py,
                 "Dirstate error".to_string(),
             )),
         }
     }
 
     def filefoldmapasdict(&self) -> PyResult<PyDict> {
         let dict = PyDict::new(py);
         for (key, value) in
             self.inner_shared(py).borrow_mut()?.build_file_fold_map().iter()
         {
             dict.set_item(py, key.as_ref().to_vec(), value.as_ref().to_vec())?;
         }
         Ok(dict)
     }
 
     def __len__(&self) -> PyResult<usize> {
         Ok(self.inner(py).borrow().len())
     }
 
     def __contains__(&self, key: PyObject) -> PyResult<bool> {
         let key = key.extract::<PyBytes>(py)?;
         Ok(self.inner(py).borrow().contains_key(HgPath::new(key.data(py))))
     }
 
     def __getitem__(&self, key: PyObject) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
         let key = HgPath::new(key.data(py));
         match self.inner(py).borrow().get(key) {
             Some(entry) => {
                 Ok(make_dirstate_tuple(py, entry)?)
             },
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
                 String::from_utf8_lossy(key.as_bytes()),
             )),
         }
     }
 
     def keys(&self) -> PyResult<DirstateMapKeysIterator> {
         let (leak_handle, leaked_ref) =
             unsafe { self.inner_shared(py).leak_immutable()? };
         DirstateMapKeysIterator::from_inner(
             py,
             leak_handle,
             leaked_ref.iter(),
         )
     }
 
     def items(&self) -> PyResult<DirstateMapItemsIterator> {
         let (leak_handle, leaked_ref) =
             unsafe { self.inner_shared(py).leak_immutable()? };
         DirstateMapItemsIterator::from_inner(
             py,
             leak_handle,
             leaked_ref.iter(),
         )
     }
 
     def __iter__(&self) -> PyResult<DirstateMapKeysIterator> {
         let (leak_handle, leaked_ref) =
             unsafe { self.inner_shared(py).leak_immutable()? };
         DirstateMapKeysIterator::from_inner(
             py,
             leak_handle,
             leaked_ref.iter(),
         )
     }
 
     def getdirs(&self) -> PyResult<Dirs> {
         // TODO don't copy, share the reference
         self.inner_shared(py).borrow_mut()?.set_dirs();
         Dirs::from_inner(
             py,
             DirsMultiset::from_dirstate(
                 &self.inner(py).borrow(),
                 Some(EntryState::Removed),
             ),
         )
     }
     def getalldirs(&self) -> PyResult<Dirs> {
         // TODO don't copy, share the reference
         self.inner_shared(py).borrow_mut()?.set_all_dirs();
         Dirs::from_inner(
             py,
             DirsMultiset::from_dirstate(
                 &self.inner(py).borrow(),
                 None,
             ),
         )
     }
 
     // TODO all copymap* methods, see docstring above
     def copymapcopy(&self) -> PyResult<PyDict> {
         let dict = PyDict::new(py);
         for (key, value) in self.inner(py).borrow().copy_map.iter() {
             dict.set_item(
                 py,
                 PyBytes::new(py, key.as_ref()),
                 PyBytes::new(py, value.as_ref()),
             )?;
         }
         Ok(dict)
     }
 
     def copymapgetitem(&self, key: PyObject) -> PyResult<PyBytes> {
         let key = key.extract::<PyBytes>(py)?;
         match self.inner(py).borrow().copy_map.get(HgPath::new(key.data(py))) {
             Some(copy) => Ok(PyBytes::new(py, copy.as_ref())),
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
                 String::from_utf8_lossy(key.data(py)),
             )),
         }
     }
     def copymap(&self) -> PyResult<CopyMap> {
         CopyMap::from_inner(py, self.clone_ref(py))
     }
 
     def copymaplen(&self) -> PyResult<usize> {
         Ok(self.inner(py).borrow().copy_map.len())
     }
     def copymapcontains(&self, key: PyObject) -> PyResult<bool> {
         let key = key.extract::<PyBytes>(py)?;
         Ok(self
             .inner(py)
             .borrow()
             .copy_map
             .contains_key(HgPath::new(key.data(py))))
     }
     def copymapget(
         &self,
         key: PyObject,
         default: Option<PyObject>
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self
             .inner(py)
             .borrow()
             .copy_map
             .get(HgPath::new(key.data(py)))
         {
             Some(copy) => Ok(Some(
                 PyBytes::new(py, copy.as_ref()).into_object(),
             )),
             None => Ok(default),
         }
     }
     def copymapsetitem(
         &self,
         key: PyObject,
         value: PyObject
     ) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
         let value = value.extract::<PyBytes>(py)?;
         self.inner_shared(py).borrow_mut()?.copy_map.insert(
             HgPathBuf::from_bytes(key.data(py)),
             HgPathBuf::from_bytes(value.data(py)),
         );
         Ok(py.None())
     }
     def copymappop(
         &self,
         key: PyObject,
         default: Option<PyObject>
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self
             .inner_shared(py)
             .borrow_mut()?
             .copy_map
             .remove(HgPath::new(key.data(py)))
         {
             Some(_) => Ok(None),
             None => Ok(default),
         }
     }
 
     def copymapiter(&self) -> PyResult<CopyMapKeysIterator> {
         let (leak_handle, leaked_ref) =
             unsafe { self.inner_shared(py).leak_immutable()? };
         CopyMapKeysIterator::from_inner(
             py,
             leak_handle,
             leaked_ref.copy_map.iter(),
         )
     }
 
     def copymapitemsiter(&self) -> PyResult<CopyMapItemsIterator> {
         let (leak_handle, leaked_ref) =
             unsafe { self.inner_shared(py).leak_immutable()? };
         CopyMapItemsIterator::from_inner(
             py,
             leak_handle,
             leaked_ref.copy_map.iter(),
         )
     }
 
 });
 
 impl DirstateMap {
+    pub fn get_inner<'a>(
+        &'a self,
+        py: Python<'a>,
+    ) -> Ref<'a, RustDirstateMap> {
+        self.inner_shared(py).borrow()
+    }
     fn translate_key(
         py: Python,
         res: (&HgPathBuf, &DirstateEntry),
     ) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, res.0.as_ref())))
     }
     fn translate_key_value(
         py: Python,
         res: (&HgPathBuf, &DirstateEntry),
     ) -> PyResult<Option<(PyBytes, PyObject)>> {
         let (f, entry) = res;
         Ok(Some((
             PyBytes::new(py, f.as_ref()),
             make_dirstate_tuple(py, entry)?,
         )))
     }
 }
 
 py_shared_ref!(DirstateMap, RustDirstateMap, inner, inner_shared);
 
 py_shared_iterator!(
     DirstateMapKeysIterator,
     PyLeakedRef,
     StateMapIter<'static>,
     DirstateMap::translate_key,
     Option<PyBytes>
 );
 
 py_shared_iterator!(
     DirstateMapItemsIterator,
     PyLeakedRef,
     StateMapIter<'static>,
     DirstateMap::translate_key_value,
     Option<(PyBytes, PyObject)>
 );
 
 fn extract_node_id(py: Python, obj: &PyObject) -> PyResult<[u8; PARENT_SIZE]> {
     let bytes = obj.extract::<PyBytes>(py)?;
     match bytes.data(py).try_into() {
         Ok(s) => Ok(s),
         Err(e) => Err(PyErr::new::<exc::ValueError, _>(py, e.to_string())),
     }
 }