Pass operations as a mutable list (#15)

This more closely resembles the Rust API, and is a sufficiently
ubiquitous concept in TaskChampion that I don't think it will be
confusing for Python programmers.

src/lib.rs

@@ -9,6 +9,8 @@ pub mod dependency_map;
 use dependency_map::*;
 pub mod operation;
 use operation::*;
+pub mod operations;
+use operations::*;
 mod task;
 use task::{Annotation, Status, Tag, Task, TaskData};
 
@@ -23,6 +25,7 @@ fn taskchampion(m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_class::<Tag>()?;
     m.add_class::<DependencyMap>()?;
     m.add_class::<Operation>()?;
+    m.add_class::<Operations>()?;
 
     Ok(())
 }

src/operation.rs

@@ -1,5 +1,5 @@
 use chrono::DateTime;
-use pyo3::prelude::*;
+use pyo3::{exceptions::PyAttributeError, prelude::*};
 
 use std::collections::HashMap;
 use taskchampion::{Operation as TCOperation, Uuid};
@@ -9,6 +9,11 @@ use taskchampion::{Operation as TCOperation, Uuid};
 pub struct Operation(pub(crate) TCOperation);
 
 #[pymethods]
+/// An Operation defines a single change to the task database, as stored locally in the replica.
+///
+/// This is an enum in Rust, represented here with four static constructors for the variants,
+/// four `is_..` methods for determining the type, and getters for each variant field. The
+/// getters raise `AttributeError` for variants that do not have the given property.
 impl Operation {
     #[allow(non_snake_case)]
     #[staticmethod]
@@ -51,9 +56,110 @@ impl Operation {
     pub fn UndoPoint() -> Operation {
         Operation(TCOperation::UndoPoint)
     }
+
+    pub fn __repr__(&self) -> String {
+        format!("{:?}", self.0)
+    }
+
+    pub fn is_create(&self) -> bool {
+        matches!(self.0, TCOperation::Create { .. })
+    }
+
+    pub fn is_delete(&self) -> bool {
+        matches!(self.0, TCOperation::Delete { .. })
+    }
+
+    pub fn is_update(&self) -> bool {
+        matches!(self.0, TCOperation::Update { .. })
+    }
+
     pub fn is_undo_point(&self) -> bool {
-        self.0.is_undo_point()
+        matches!(self.0, TCOperation::UndoPoint)
+    }
+
+    #[getter(uuid)]
+    pub fn get_uuid(&self) -> PyResult<String> {
+        use TCOperation::*;
+        match &self.0 {
+            Create { uuid } => Ok(uuid.to_string()),
+            Delete { uuid, .. } => Ok(uuid.to_string()),
+            Update { uuid, .. } => Ok(uuid.to_string()),
+            _ => Err(PyAttributeError::new_err(
+                "Variant does not have attribute 'uuid'",
+            )),
+        }
+    }
+
+    #[getter(old_task)]
+    pub fn get_old_task(&self) -> PyResult<HashMap<String, String>> {
+        use TCOperation::*;
+        match &self.0 {
+            Delete { old_task, .. } => Ok(old_task.clone()),
+            _ => Err(PyAttributeError::new_err(
+                "Variant does not have attribute 'old_task'",
+            )),
+        }
+    }
+
+    #[getter(property)]
+    pub fn get_property(&self) -> PyResult<String> {
+        use TCOperation::*;
+        match &self.0 {
+            Update { property, .. } => Ok(property.clone()),
+            _ => Err(PyAttributeError::new_err(
+                "Variant does not have attribute 'property'",
+            )),
+        }
+    }
+
+    #[getter(timestamp)]
+    pub fn get_timestamp(&self) -> PyResult<String> {
+        use TCOperation::*;
+        match &self.0 {
+            Update { timestamp, .. } => Ok(timestamp.to_string()),
+            _ => Err(PyAttributeError::new_err(
+                "Variant does not have attribute 'timestamp'",
+            )),
+        }
+    }
+
+    #[getter(old_value)]
+    pub fn get_old_value(&self) -> PyResult<Option<String>> {
+        use TCOperation::*;
+        match &self.0 {
+            Update { old_value, .. } => Ok(old_value.clone()),
+            _ => Err(PyAttributeError::new_err(
+                "Variant does not have attribute 'old_value'",
+            )),
+        }
+    }
+
+    #[getter(value)]
+    pub fn get_value(&self) -> PyResult<Option<String>> {
+        use TCOperation::*;
+        match &self.0 {
+            Update { value, .. } => Ok(value.clone()),
+            _ => Err(PyAttributeError::new_err(
+                "Variant does not have attribute 'value'",
+            )),
+        }
+    }
+}
+
+impl AsRef<TCOperation> for Operation {
+    fn as_ref(&self) -> &TCOperation {
+        &self.0
     }
 }
 
-pub type Operations = Vec<Operation>;
+impl AsMut<TCOperation> for Operation {
+    fn as_mut(&mut self) -> &mut TCOperation {
+        &mut self.0
+    }
+}
+
+impl From<Operation> for TCOperation {
+    fn from(val: Operation) -> Self {
+        val.0
+    }
+}

src/operations.rs

@@ -0,0 +1,58 @@
+use crate::Operation;
+use pyo3::{exceptions::PyIndexError, prelude::*};
+use taskchampion::Operations as TCOperations;
+
+#[pyclass(sequence)]
+#[derive(PartialEq, Eq, Clone, Debug)]
+pub struct Operations(TCOperations);
+
+#[pymethods]
+impl Operations {
+    #[new]
+    pub fn new() -> Operations {
+        Operations(TCOperations::new())
+    }
+
+    pub fn append(&mut self, op: Operation) {
+        self.0.push(op.into());
+    }
+
+    pub fn __repr__(&self) -> String {
+        format!("{:?}", self)
+    }
+
+    pub fn __len__(&self) -> usize {
+        self.0.len()
+    }
+
+    pub fn __getitem__(&self, i: usize) -> PyResult<Operation> {
+        if i >= self.0.len() {
+            return Err(PyIndexError::new_err("Invalid operation index"));
+        }
+        Ok(Operation(self.0[i].clone()))
+    }
+}
+
+impl AsRef<TCOperations> for Operations {
+    fn as_ref(&self) -> &TCOperations {
+        &self.0
+    }
+}
+
+impl AsMut<TCOperations> for Operations {
+    fn as_mut(&mut self) -> &mut TCOperations {
+        &mut self.0
+    }
+}
+
+impl From<Operations> for TCOperations {
+    fn from(val: Operations) -> Self {
+        val.0
+    }
+}
+
+impl From<TCOperations> for Operations {
+    fn from(val: TCOperations) -> Self {
+        Operations(val)
+    }
+}

src/replica.rs

@@ -2,11 +2,9 @@ use std::collections::HashMap;
 use std::rc::Rc;
 
 use crate::task::TaskData;
-use crate::{DependencyMap, Operation, Task, WorkingSet};
+use crate::{DependencyMap, Operations, Task, WorkingSet};
 use pyo3::prelude::*;
-use taskchampion::{
-    Operations as TCOperations, Replica as TCReplica, ServerConfig, StorageConfig, Uuid,
-};
+use taskchampion::{Replica as TCReplica, ServerConfig, StorageConfig, Uuid};
 
 #[pyclass]
 /// A replica represents an instance of a user's task data, providing an easy interface
@@ -43,13 +41,12 @@ impl Replica {
 
     /// Create a new task
     /// The task must not already exist.
-    pub fn create_task(&mut self, uuid: String) -> anyhow::Result<(Task, Vec<Operation>)> {
-        let mut ops = TCOperations::new();
+    pub fn create_task(&mut self, uuid: String, ops: &mut Operations) -> anyhow::Result<Task> {
         let task = self
             .0
-            .create_task(Uuid::parse_str(&uuid)?, &mut ops)
+            .create_task(Uuid::parse_str(&uuid)?, ops.as_mut())
             .map(Task)?;
-        Ok((task, ops.iter().map(|op| Operation(op.clone())).collect()))
+        Ok(task)
     }
 
     /// Get a list of all tasks in the replica.
@@ -121,6 +118,10 @@ impl Replica {
         Ok(self.0.sync(&mut server, avoid_snapshots)?)
     }
 
+    pub fn commit_operations(&mut self, ops: Operations) -> anyhow::Result<()> {
+        Ok(self.0.commit_operations(ops.into())?)
+    }
+
     /// Sync with a server created from `ServerConfig::Remote`.
     fn sync_to_remote(
         &mut self,
@@ -156,13 +157,10 @@ impl Replica {
         Ok(self.0.sync(&mut server, avoid_snapshots)?)
     }
 
-    pub fn commit_operations(&mut self, operations: Vec<Operation>) -> anyhow::Result<()> {
-        let ops = operations.iter().map(|op| op.0.clone()).collect();
-        Ok(self.0.commit_operations(ops)?)
-    }
     pub fn rebuild_working_set(&mut self, renumber: bool) -> anyhow::Result<()> {
         Ok(self.0.rebuild_working_set(renumber)?)
     }
+
     pub fn num_local_operations(&mut self) -> anyhow::Result<usize> {
         Ok(self.0.num_local_operations()?)
     }
@@ -171,20 +169,12 @@ impl Replica {
         Ok(self.0.num_local_operations()?)
     }
 
-    pub fn get_undo_operations(&mut self) -> anyhow::Result<Vec<Operation>> {
-        Ok(self
-            .0
-            .get_undo_operations()
-            .map(|ops| ops.iter().map(|op| Operation(op.clone())).collect())?)
+    pub fn get_undo_operations(&mut self) -> anyhow::Result<Operations> {
+        Ok(self.0.get_undo_operations()?.into())
     }
 
-    pub fn commit_reversed_operations(
-        &mut self,
-        operations: Vec<Operation>,
-    ) -> anyhow::Result<bool> {
-        let ops = operations.iter().map(|op| op.0.clone()).collect();
-
-        Ok(self.0.commit_reversed_operations(ops)?)
+    pub fn commit_reversed_operations(&mut self, operations: Operations) -> anyhow::Result<bool> {
+        Ok(self.0.commit_reversed_operations(operations.into())?)
     }
 
     pub fn expire_tasks(&mut self) -> anyhow::Result<()> {

src/task/data.rs

@@ -1,22 +1,19 @@
-use crate::Operation;
-use pyo3::prelude::*;
-use taskchampion::{Operation as TCOperation, TaskData as TCTaskData, Uuid};
+use crate::Operations;
+use pyo3::{exceptions::PyValueError, prelude::*};
+use taskchampion::{TaskData as TCTaskData, Uuid};
 
 #[pyclass]
 pub struct TaskData(pub(crate) TCTaskData);
 
 #[pymethods]
 impl TaskData {
     #[staticmethod]
-    pub fn create(uuid: String) -> (Self, Operation) {
-        let u = Uuid::parse_str(&uuid).expect("invalid UUID");
-
-        let mut ops: Vec<TCOperation> = vec![TCOperation::Create { uuid: u }];
-
-        let td = TaskData(TCTaskData::create(u, &mut ops));
-        (td, Operation(ops.first().expect("").clone()))
+    pub fn create(uuid: String, ops: &mut Operations) -> PyResult<Self> {
+        let u = Uuid::parse_str(&uuid).map_err(|_| PyValueError::new_err("Invalid UUID"))?;
+        Ok(TaskData(TCTaskData::create(u, ops.as_mut())))
     }
 
+    #[getter(uuid)]
     pub fn get_uuid(&self) -> String {
         self.0.get_uuid().into()
     }
@@ -29,18 +26,12 @@ impl TaskData {
         self.0.has(value)
     }
 
-    #[pyo3(signature=(property, value=None))]
-    pub fn update(&mut self, property: String, value: Option<String>) -> Operation {
-        let mut ops: Vec<TCOperation> = Vec::new();
-
-        self.0.update(property, value, &mut ops);
-        ops.first().map(|op| Operation(op.clone())).expect("")
+    #[pyo3(signature=(property, value, ops))]
+    pub fn update(&mut self, property: String, value: Option<String>, ops: &mut Operations) {
+        self.0.update(property, value, ops.as_mut());
     }
 
-    pub fn delete(&mut self) -> Operation {
-        let mut ops: Vec<TCOperation> = Vec::new();
-        self.0.delete(&mut ops);
-
-        ops.first().map(|op| Operation(op.clone())).expect("")
+    pub fn delete(&mut self, ops: &mut Operations) {
+        self.0.delete(ops.as_mut());
     }
 }