Merge pull request #147 from djmitche/issue144

Reorganize the 'taskchampion' crate

cli/src/invocation/mod.rs

@@ -3,7 +3,7 @@
 use crate::argparse::{Command, Subcommand};
 use config::Config;
 use failure::{format_err, Fallible};
-use taskchampion::{server, Replica, ReplicaConfig, ServerConfig, Uuid};
+use taskchampion::{Replica, Server, ServerConfig, StorageConfig, Uuid};
 use termcolor::{ColorChoice, StandardStream};
 
 mod cmd;
@@ -104,15 +104,15 @@ pub(crate) fn invoke(command: Command, settings: Config) -> Fallible<()> {
 fn get_replica(settings: &Config) -> Fallible<Replica> {
     let taskdb_dir = settings.get_str("data_dir")?.into();
     log::debug!("Replica data_dir: {:?}", taskdb_dir);
-    let replica_config = ReplicaConfig { taskdb_dir };
+    let storage_config = StorageConfig::OnDisk { taskdb_dir };
-    Ok(Replica::from_config(replica_config)?)
+    Ok(Replica::new(storage_config.into_storage()?))
 }
 
 /// Get the server for this invocation
-fn get_server(settings: &Config) -> Fallible<Box<dyn server::Server>> {
+fn get_server(settings: &Config) -> Fallible<Box<dyn Server>> {
     // if server_client_key and server_origin are both set, use
     // the remote server
-    if let (Ok(client_key), Ok(origin)) = (
+    let config = if let (Ok(client_key), Ok(origin)) = (
         settings.get_str("server_client_key"),
         settings.get_str("server_origin"),
     ) {
@@ -123,16 +123,17 @@ fn get_server(settings: &Config) -> Fallible<Box<dyn server::Server>> {
 
         log::debug!("Using sync-server with origin {}", origin);
         log::debug!("Sync client ID: {}", client_key);
-        Ok(server::from_config(ServerConfig::Remote {
+        ServerConfig::Remote {
             origin,
             client_key,
             encryption_secret: encryption_secret.as_bytes().to_vec(),
-        })?)
+        }
     } else {
         let server_dir = settings.get_str("server_dir")?.into();
         log::debug!("Using local sync-server at `{:?}`", server_dir);
-        Ok(server::from_config(ServerConfig::Local { server_dir })?)
+        ServerConfig::Local { server_dir }
-    }
+    };
+    Ok(config.into_server()?)
 }
 
 /// Get a WriteColor implementation based on whether the output is a tty.

cli/src/invocation/test.rs

@@ -1,16 +1,17 @@
 use std::io;
-use taskchampion::{server, taskstorage, Replica, ServerConfig};
+use taskchampion::{storage, Replica, Server, ServerConfig};
 use tempdir::TempDir;
 
 pub(super) fn test_replica() -> Replica {
-    let storage = taskstorage::InMemoryStorage::new();
+    let storage = storage::InMemoryStorage::new();
     Replica::new(Box::new(storage))
 }
 
-pub(super) fn test_server(dir: &TempDir) -> Box<dyn server::Server> {
+pub(super) fn test_server(dir: &TempDir) -> Box<dyn Server> {
-    server::from_config(ServerConfig::Local {
+    ServerConfig::Local {
         server_dir: dir.path().to_path_buf(),
-    })
+    }
+    .into_server()
     .unwrap()
 }
 

docs/src/storage.md

@@ -1,7 +1,7 @@
 # Replica Storage
 
 Each replica has a storage backend.
-The interface for this backend is given in `crate::taskstorage::TaskStorage` and `TaskStorageTxn`.
+The interface for this backend is given in `crate::taskstorage::Storage` and `StorageTxn`.
 
 The storage is transaction-protected, with the expectation of a serializable isolation level.
 The storage contains the following information:

taskchampion/src/lib.rs

@@ -3,19 +3,26 @@
 
 This crate implements the core of TaskChampion, the [replica](crate::Replica).
 
+# Replica
+
 A TaskChampion replica is a local copy of a user's task data.  As the name suggests, several
 replicas of the same data can exist (such as on a user's laptop and on their phone) and can
 synchronize with one another.
 
+Replicas are accessed using the [`Replica`](crate::replica) type.
+
 # Task Storage
 
-The [`taskstorage`](crate::taskstorage) module supports pluggable storage for a replica's data.
+The [`storage`](crate::storage) module supports pluggable storage for a replica's data.
 An implementation is provided, but users of this crate can provide their own implementation as well.
 
 # Server
 
 Replica synchronization takes place against a server.
+Create a server with [`ServerConfig`](crate::ServerConfig).
+
 The [`server`](crate::server) module defines the interface a server must meet.
+Users can define their own server impelementations.
 
 # See Also
 
@@ -24,18 +31,18 @@ for more information about the design and usage of the tool.
 
  */
 
-mod config;
 mod errors;
 mod replica;
 pub mod server;
+pub mod storage;
 mod task;
 mod taskdb;
-pub mod taskstorage;
 mod utils;
 mod workingset;
 
-pub use config::{ReplicaConfig, ServerConfig};
 pub use replica::Replica;
+pub use server::{Server, ServerConfig};
+pub use storage::StorageConfig;
 pub use task::{Priority, Status, Tag, Task, TaskMut};
 pub use workingset::WorkingSet;
 

taskchampion/src/replica.rs

@@ -1,9 +1,8 @@
-use crate::config::ReplicaConfig;
 use crate::errors::Error;
 use crate::server::Server;
+use crate::storage::{Operation, Storage, TaskMap};
 use crate::task::{Status, Task};
 use crate::taskdb::TaskDB;
-use crate::taskstorage::{KVStorage, Operation, TaskMap, TaskStorage};
 use crate::workingset::WorkingSet;
 use chrono::Utc;
 use failure::Fallible;
@@ -30,22 +29,15 @@ pub struct Replica {
 }
 
 impl Replica {
-    pub fn new(storage: Box<dyn TaskStorage>) -> Replica {
+    pub fn new(storage: Box<dyn Storage>) -> Replica {
         Replica {
             taskdb: TaskDB::new(storage),
         }
     }
 
-    /// Construct a new replica from a configuration object.  This is the common way
-    /// to create a new object.
-    pub fn from_config(config: ReplicaConfig) -> Fallible<Replica> {
-        let storage = Box::new(KVStorage::new(config.taskdb_dir)?);
-        Ok(Replica::new(storage))
-    }
-
     #[cfg(test)]
     pub fn new_inmemory() -> Replica {
-        Replica::new(Box::new(crate::taskstorage::InMemoryStorage::new()))
+        Replica::new(Box::new(crate::storage::InMemoryStorage::new()))
     }
 
     /// Update an existing task.  If the value is Some, the property is added or updated.  If the

taskchampion/src/server/config.rs

@@ -1,14 +1,10 @@
+use super::types::Server;
+use super::{LocalServer, RemoteServer};
+use failure::Fallible;
 use std::path::PathBuf;
 use uuid::Uuid;
 
-/// The configuration required for a replica.  Use with [`crate::Replica::from_config`].
+/// The configuration for a replica's access to a sync server.
-pub struct ReplicaConfig {
-    /// Path containing the task DB.
-    pub taskdb_dir: PathBuf,
-}
-
-/// The configuration for a replica's access to a sync server.  Use with
-/// [`crate::server::from_config`].
 pub enum ServerConfig {
     /// A local task database, for situations with a single replica.
     Local {
@@ -28,3 +24,17 @@ pub enum ServerConfig {
         encryption_secret: Vec<u8>,
     },
 }
+
+impl ServerConfig {
+    /// Get a server based on this configuration
+    pub fn into_server(self) -> Fallible<Box<dyn Server>> {
+        Ok(match self {
+            ServerConfig::Local { server_dir } => Box::new(LocalServer::new(server_dir)?),
+            ServerConfig::Remote {
+                origin,
+                client_key,
+                encryption_secret,
+            } => Box::new(RemoteServer::new(origin, client_key, encryption_secret)),
+        })
+    }
+}

taskchampion/src/server/mod.rs

@@ -1,25 +1,22 @@
-use crate::ServerConfig;
+/**
-use failure::Fallible;
+
+This module defines the client interface to TaskChampion sync servers.
+It defines a [trait](crate::server::Server) for servers, and implements both local and remote servers.
+
+Typical uses of this crate do not interact directly with this module; [`ServerConfig`](crate::ServerConfig) is sufficient.
+However, users who wish to implement their own server interfaces can implement the traits defined here and pass the result to [`Replica`](crate::Replica).
+
+*/
 
 #[cfg(test)]
 pub(crate) mod test;
 
+mod config;
 mod local;
 mod remote;
 mod types;
 
+pub use config::ServerConfig;
 pub use local::LocalServer;
 pub use remote::RemoteServer;
 pub use types::*;
-
-/// Create a new server based on the given configuration.
-pub fn from_config(config: ServerConfig) -> Fallible<Box<dyn Server>> {
-    Ok(match config {
-        ServerConfig::Local { server_dir } => Box::new(LocalServer::new(server_dir)?),
-        ServerConfig::Remote {
-            origin,
-            client_key,
-            encryption_secret,
-        } => Box::new(RemoteServer::new(origin, client_key, encryption_secret)),
-    })
-}

taskchampion/src/storage/config.rs

@@ -0,0 +1,23 @@
+use super::{InMemoryStorage, KVStorage, Storage};
+use failure::Fallible;
+use std::path::PathBuf;
+
+/// The configuration required for a replica's storage.
+pub enum StorageConfig {
+    /// Store the data on disk.  This is the common choice.
+    OnDisk {
+        /// Path containing the task DB.
+        taskdb_dir: PathBuf,
+    },
+    /// Store the data in memory.  This is only useful for testing.
+    InMemory,
+}
+
+impl StorageConfig {
+    pub fn into_storage(self) -> Fallible<Box<dyn Storage>> {
+        Ok(match self {
+            StorageConfig::OnDisk { taskdb_dir } => Box::new(KVStorage::new(taskdb_dir)?),
+            StorageConfig::InMemory => Box::new(InMemoryStorage::new()),
+        })
+    }
+}

taskchampion/src/storage/inmemory.rs

@@ -1,8 +1,6 @@
 #![allow(clippy::new_without_default)]
 
-use crate::taskstorage::{
+use crate::storage::{Operation, Storage, StorageTxn, TaskMap, VersionId, DEFAULT_BASE_VERSION};
-    Operation, TaskMap, TaskStorage, TaskStorageTxn, VersionId, DEFAULT_BASE_VERSION,
-};
 use failure::{bail, Fallible};
 use std::collections::hash_map::Entry;
 use std::collections::HashMap;
@@ -42,7 +40,7 @@ impl<'t> Txn<'t> {
     }
 }
 
-impl<'t> TaskStorageTxn for Txn<'t> {
+impl<'t> StorageTxn for Txn<'t> {
     fn get_task(&mut self, uuid: Uuid) -> Fallible<Option<TaskMap>> {
         match self.data_ref().tasks.get(&uuid) {
             None => Ok(None),
@@ -157,8 +155,8 @@ impl InMemoryStorage {
     }
 }
 
-impl TaskStorage for InMemoryStorage {
+impl Storage for InMemoryStorage {
-    fn txn<'a>(&'a mut self) -> Fallible<Box<dyn TaskStorageTxn + 'a>> {
+    fn txn<'a>(&'a mut self) -> Fallible<Box<dyn StorageTxn + 'a>> {
         Ok(Box::new(Txn {
             storage: self,
             new_data: None,

taskchampion/src/storage/kv.rs

@@ -1,6 +1,4 @@
-use crate::taskstorage::{
+use crate::storage::{Operation, Storage, StorageTxn, TaskMap, VersionId, DEFAULT_BASE_VERSION};
-    Operation, TaskMap, TaskStorage, TaskStorageTxn, VersionId, DEFAULT_BASE_VERSION,
-};
 use crate::utils::Key;
 use failure::{bail, Fallible};
 use kv::msgpack::Msgpack;
@@ -62,8 +60,8 @@ impl<'t> KVStorage<'t> {
     }
 }
 
-impl<'t> TaskStorage for KVStorage<'t> {
+impl<'t> Storage for KVStorage<'t> {
-    fn txn<'a>(&'a mut self) -> Fallible<Box<dyn TaskStorageTxn + 'a>> {
+    fn txn<'a>(&'a mut self) -> Fallible<Box<dyn StorageTxn + 'a>> {
         Ok(Box::new(Txn {
             storage: self,
             txn: Some(self.store.write_txn()?),
@@ -104,7 +102,7 @@ impl<'t> Txn<'t> {
     }
 }
 
-impl<'t> TaskStorageTxn for Txn<'t> {
+impl<'t> StorageTxn for Txn<'t> {
     fn get_task(&mut self, uuid: Uuid) -> Fallible<Option<TaskMap>> {
         let bucket = self.tasks_bucket();
         let buf = match self.kvtxn().get(bucket, uuid.into()) {
@@ -356,7 +354,7 @@ impl<'t> TaskStorageTxn for Txn<'t> {
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::taskstorage::taskmap_with;
+    use crate::storage::taskmap_with;
     use failure::Fallible;
     use tempdir::TempDir;
 

taskchampion/src/storage/mod.rs

@@ -1,12 +1,23 @@
+/**
+
+This module defines the backend storage used by [`Replica`](crate::Replica).
+It defines a [trait](crate::storage::Storage) for storage implementations, and provides a default on-disk implementation as well as an in-memory implementation for testing.
+
+Typical uses of this crate do not interact directly with this module; [`StorageConfig`](crate::StorageConfig) is sufficient.
+However, users who wish to implement their own storage backends can implement the traits defined here and pass the result to [`Replica`](crate::Replica).
+
+*/
 use failure::Fallible;
 use std::collections::HashMap;
 use uuid::Uuid;
 
+mod config;
 mod inmemory;
 mod kv;
 mod operation;
 
 pub use self::kv::KVStorage;
+pub use config::StorageConfig;
 pub use inmemory::InMemoryStorage;
 
 pub use operation::Operation;
@@ -29,7 +40,7 @@ pub use crate::server::VersionId;
 /// The default for base_version.
 pub(crate) const DEFAULT_BASE_VERSION: Uuid = crate::server::NO_VERSION_ID;
 
-/// A TaskStorage transaction, in which storage operations are performed.
+/// A Storage transaction, in which storage operations are performed.
 ///
 /// # Concurrency
 ///
@@ -40,9 +51,9 @@ pub(crate) const DEFAULT_BASE_VERSION: Uuid = crate::server::NO_VERSION_ID;
 /// # Commiting and Aborting
 ///
 /// A transaction is not visible to other readers until it is committed with
-/// [`crate::taskstorage::TaskStorageTxn::commit`].  Transactions are aborted if they are dropped.
+/// [`crate::storage::StorageTxn::commit`].  Transactions are aborted if they are dropped.
 /// It is safe and performant to drop transactions that did not modify any data without committing.
-pub trait TaskStorageTxn {
+pub trait StorageTxn {
     /// Get an (immutable) task, if it is in the storage
     fn get_task(&mut self, uuid: Uuid) -> Fallible<Option<TaskMap>>;
 
@@ -102,8 +113,8 @@ pub trait TaskStorageTxn {
 }
 
 /// A trait for objects able to act as task storage.  Most of the interesting behavior is in the
-/// [`crate::taskstorage::TaskStorageTxn`] trait.
+/// [`crate::storage::StorageTxn`] trait.
-pub trait TaskStorage {
+pub trait Storage {
     /// Begin a transaction
-    fn txn<'a>(&'a mut self) -> Fallible<Box<dyn TaskStorageTxn + 'a>>;
+    fn txn<'a>(&'a mut self) -> Fallible<Box<dyn StorageTxn + 'a>>;
 }

taskchampion/src/storage/operation.rs

@@ -125,8 +125,8 @@ impl Operation {
 #[cfg(test)]
 mod test {
     use super::*;
+    use crate::storage::InMemoryStorage;
     use crate::taskdb::TaskDB;
-    use crate::taskstorage::InMemoryStorage;
     use chrono::{Duration, Utc};
     use proptest::prelude::*;
 

taskchampion/src/task.rs

@@ -1,5 +1,5 @@
 use crate::replica::Replica;
-use crate::taskstorage::TaskMap;
+use crate::storage::TaskMap;
 use chrono::prelude::*;
 use failure::{format_err, Fallible};
 use log::trace;

taskchampion/src/taskdb.rs

@@ -1,6 +1,6 @@
 use crate::errors::Error;
 use crate::server::{AddVersionResult, GetVersionResult, Server};
-use crate::taskstorage::{Operation, TaskMap, TaskStorage, TaskStorageTxn};
+use crate::storage::{Operation, Storage, StorageTxn, TaskMap};
 use failure::{format_err, Fallible};
 use log::{info, trace, warn};
 use serde::{Deserialize, Serialize};
@@ -12,7 +12,7 @@ use uuid::Uuid;
 /// and so on, and all the invariants that come with it.  It leaves the meaning of particular task
 /// properties to the replica and task implementations.
 pub struct TaskDB {
-    storage: Box<dyn TaskStorage>,
+    storage: Box<dyn Storage>,
 }
 
 #[derive(Serialize, Deserialize, Debug)]
@@ -22,13 +22,13 @@ struct Version {
 
 impl TaskDB {
     /// Create a new TaskDB with the given backend storage
-    pub fn new(storage: Box<dyn TaskStorage>) -> TaskDB {
+    pub fn new(storage: Box<dyn Storage>) -> TaskDB {
         TaskDB { storage }
     }
 
     #[cfg(test)]
     pub fn new_inmemory() -> TaskDB {
-        TaskDB::new(Box::new(crate::taskstorage::InMemoryStorage::new()))
+        TaskDB::new(Box::new(crate::storage::InMemoryStorage::new()))
     }
 
     /// Apply an operation to the TaskDB.  Aside from synchronization operations, this is the only way
@@ -45,7 +45,7 @@ impl TaskDB {
         Ok(())
     }
 
-    fn apply_op(txn: &mut dyn TaskStorageTxn, op: &Operation) -> Fallible<()> {
+    fn apply_op(txn: &mut dyn StorageTxn, op: &Operation) -> Fallible<()> {
         match op {
             Operation::Create { uuid } => {
                 // insert if the task does not already exist
@@ -261,7 +261,7 @@ impl TaskDB {
         Ok(())
     }
 
-    fn apply_version(txn: &mut dyn TaskStorageTxn, mut version: Version) -> Fallible<()> {
+    fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> Fallible<()> {
         // The situation here is that the server has already applied all server operations, and we
         // have already applied all local operations, so states have diverged by several
         // operations.  We need to figure out what operations to apply locally and on the server in
@@ -358,7 +358,7 @@ impl TaskDB {
 mod tests {
     use super::*;
     use crate::server::test::TestServer;
-    use crate::taskstorage::InMemoryStorage;
+    use crate::storage::InMemoryStorage;
     use chrono::Utc;
     use proptest::prelude::*;
     use std::collections::HashMap;