Skip to content

Commit

Permalink
Consistent naming of local trait implementations
Browse files Browse the repository at this point in the history
Call them LocalBlacklist, etc., in contrast to possible remote
implementations.

Use dynamic dispatch in SchedulerData instead of implementations.
No significant performance degradation observed in actual testing.
  • Loading branch information
martinmr committed Apr 12, 2024
1 parent f350419 commit 4ae2992
Show file tree
Hide file tree
Showing 11 changed files with 61 additions and 68 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "trane"
version = "0.19.2"
version = "0.20.0"
edition = "2021"
description = "An automated system for learning complex skills"
license = "AGPL-3.0"
Expand Down
20 changes: 10 additions & 10 deletions src/blacklist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,15 @@ pub trait Blacklist {
}

/// An implementation of [Blacklist] backed by `SQLite`.
pub struct BlacklistDB {
pub struct LocalBlacklist {
/// A cache of the blacklist entries used to avoid unnecessary queries to the database.
cache: RwLock<UstrMap<bool>>,

/// A pool of connections to the database.
pool: Pool<SqliteConnectionManager>,
}

impl BlacklistDB {
impl LocalBlacklist {
/// Returns all the migrations needed to set up the database.
fn migrations() -> Migrations<'static> {
Migrations::new(vec![
Expand All @@ -68,10 +68,10 @@ impl BlacklistDB {
}

/// A constructor taking a connection manager.
fn new(connection_manager: SqliteConnectionManager) -> Result<BlacklistDB> {
fn new(connection_manager: SqliteConnectionManager) -> Result<LocalBlacklist> {
// Initialize the pool and the blacklist database.
let pool = Pool::new(connection_manager)?;
let mut blacklist = BlacklistDB {
let mut blacklist = LocalBlacklist {
cache: RwLock::new(UstrMap::default()),
pool,
};
Expand All @@ -86,7 +86,7 @@ impl BlacklistDB {
}

/// A constructor taking the path to the database file.
pub fn new_from_disk(db_path: &str) -> Result<BlacklistDB> {
pub fn new_from_disk(db_path: &str) -> Result<LocalBlacklist> {
Self::new(db_utils::new_connection_manager(db_path))
}

Expand Down Expand Up @@ -175,7 +175,7 @@ impl BlacklistDB {
}
}

impl Blacklist for BlacklistDB {
impl Blacklist for LocalBlacklist {
fn add_to_blacklist(&mut self, unit_id: Ustr) -> Result<(), BlacklistError> {
self.add_to_blacklist_helper(unit_id)
.map_err(|e| BlacklistError::AddUnit(unit_id, e))
Expand Down Expand Up @@ -209,11 +209,11 @@ mod test {
use tempfile::tempdir;
use ustr::Ustr;

use crate::blacklist::{Blacklist, BlacklistDB};
use crate::blacklist::{Blacklist, LocalBlacklist};

fn new_test_blacklist() -> Result<Box<dyn Blacklist>> {
let connection_manager = SqliteConnectionManager::memory();
let blacklist = BlacklistDB::new(connection_manager)?;
let blacklist = LocalBlacklist::new(connection_manager)?;
Ok(Box::new(blacklist))
}

Expand Down Expand Up @@ -315,13 +315,13 @@ mod test {
fn reopen_blacklist() -> Result<()> {
let dir = tempdir()?;
let mut blacklist =
BlacklistDB::new_from_disk(dir.path().join("blacklist.db").to_str().unwrap())?;
LocalBlacklist::new_from_disk(dir.path().join("blacklist.db").to_str().unwrap())?;
let unit_id = Ustr::from("unit_id");
blacklist.add_to_blacklist(unit_id)?;
assert!(blacklist.blacklisted(unit_id)?);

let new_blacklist =
BlacklistDB::new_from_disk(dir.path().join("blacklist.db").to_str().unwrap())?;
LocalBlacklist::new_from_disk(dir.path().join("blacklist.db").to_str().unwrap())?;
assert!(new_blacklist.blacklisted(unit_id)?);
Ok(())
}
Expand Down
6 changes: 3 additions & 3 deletions src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ pub struct TraneFFI {
}

impl TraneFFI {
/// Creates and wraps a new instance of Trane for use with FFI.
pub fn new(working_dir: &Path, library_root: &Path) -> Result<TraneFFI> {
/// Creates and wraps a new local instance of Trane for use with FFI.
pub fn new_local(working_dir: &Path, library_root: &Path) -> Result<TraneFFI> {
Ok(TraneFFI {
trane: Trane::new(working_dir, library_root)?,
trane: Trane::new_local(working_dir, library_root)?,
})
}
}
Expand Down
32 changes: 16 additions & 16 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ pub mod testutil;
use anyhow::Result;
use error::*;
use parking_lot::RwLock;
use review_list::{ReviewList, ReviewListDB};
use review_list::{LocalReviewList, ReviewList};
use std::{path::Path, sync::Arc};
use study_session_manager::{LocalStudySessionManager, StudySessionManager};
use ustr::{Ustr, UstrMap, UstrSet};

use crate::mantra_miner::TraneMantraMiner;
use blacklist::{Blacklist, BlacklistDB};
use blacklist::{Blacklist, LocalBlacklist};
use course_library::{CourseLibrary, GetUnitGraph, LocalCourseLibrary};
use data::{
filter::{ExerciseFilter, SavedFilter},
Expand All @@ -80,7 +80,7 @@ use data::{
};
use filter_manager::{FilterManager, LocalFilterManager};
use graph::UnitGraph;
use practice_stats::{PracticeStats, PracticeStatsDB};
use practice_stats::{LocalPracticeStats, PracticeStats};
use repository_manager::{LocalRepositoryManager, RepositoryManager};
use scheduler::{data::SchedulerData, DepthFirstScheduler, ExerciseScheduler};

Expand Down Expand Up @@ -169,10 +169,10 @@ impl Trane {
options
}

/// Creates a new instance of the library given the path to the root of a course library. The
/// user data will be stored in a directory named `.trane` inside the library root directory.
/// The working directory will be used to resolve relative paths.
pub fn new(working_dir: &Path, library_root: &Path) -> Result<Trane> {
/// Creates a new local instance of the Trane given the path to the root of a course library.
/// The user data will be stored in a directory named `.trane` inside the library root
/// directory. The working directory will be used to resolve relative paths.
pub fn new_local(working_dir: &Path, library_root: &Path) -> Result<Trane> {
let config_path = library_root.join(Path::new(TRANE_CONFIG_DIR_PATH));

// The course library must be created first because it makes sure to initialize all the
Expand All @@ -183,13 +183,13 @@ impl Trane {

// Build all the components needed to create a Trane instance.
let unit_graph = course_library.write().get_unit_graph();
let practice_stats = Arc::new(RwLock::new(PracticeStatsDB::new_from_disk(
let practice_stats = Arc::new(RwLock::new(LocalPracticeStats::new_from_disk(
config_path.join(PRACTICE_STATS_PATH).to_str().unwrap(),
)?));
let blacklist = Arc::new(RwLock::new(BlacklistDB::new_from_disk(
let blacklist = Arc::new(RwLock::new(LocalBlacklist::new_from_disk(
config_path.join(BLACKLIST_PATH).to_str().unwrap(),
)?));
let review_list = Arc::new(RwLock::new(ReviewListDB::new_from_disk(
let review_list = Arc::new(RwLock::new(LocalReviewList::new_from_disk(
config_path.join(REVIEW_LIST_PATH).to_str().unwrap(),
)?));
let filter_manager = Arc::new(RwLock::new(LocalFilterManager::new(
Expand Down Expand Up @@ -561,7 +561,7 @@ mod test {
#[test]
fn library_root() -> Result<()> {
let dir = tempfile::tempdir()?;
let trane = Trane::new(dir.path(), dir.path())?;
let trane = Trane::new_local(dir.path(), dir.path())?;
assert_eq!(trane.library_root(), dir.path().to_str().unwrap());
Ok(())
}
Expand All @@ -570,7 +570,7 @@ mod test {
#[test]
fn mantra_count() -> Result<()> {
let dir = tempfile::tempdir()?;
let trane = Trane::new(dir.path(), dir.path())?;
let trane = Trane::new_local(dir.path(), dir.path())?;
thread::sleep(Duration::from_millis(1000));
assert!(trane.mantra_count() > 0);
Ok(())
Expand All @@ -583,7 +583,7 @@ mod test {
let dir = tempfile::tempdir()?;
let trane_path = dir.path().join(".trane");
File::create(&trane_path)?;
assert!(Trane::new(dir.path(), dir.path()).is_err());
assert!(Trane::new_local(dir.path(), dir.path()).is_err());
Ok(())
}

Expand All @@ -592,7 +592,7 @@ mod test {
fn bad_dir_permissions() -> Result<()> {
let dir = tempfile::tempdir()?;
set_permissions(&dir, Permissions::from_mode(0o000))?;
assert!(Trane::new(dir.path(), dir.path()).is_err());
assert!(Trane::new_local(dir.path(), dir.path()).is_err());
Ok(())
}

Expand All @@ -603,15 +603,15 @@ mod test {
let config_dir_path = dir.path().join(".trane");
create_dir(&config_dir_path)?;
set_permissions(&config_dir_path, Permissions::from_mode(0o000))?;
assert!(Trane::new(dir.path(), dir.path()).is_err());
assert!(Trane::new_local(dir.path(), dir.path()).is_err());
Ok(())
}

/// Verifies retrieving the scheduler data from the library.
#[test]
fn scheduler_data() -> Result<()> {
let dir = tempfile::tempdir()?;
let trane = Trane::new(dir.path(), dir.path())?;
let trane = Trane::new_local(dir.path(), dir.path())?;
trane.get_scheduler_data();
Ok(())
}
Expand Down
16 changes: 8 additions & 8 deletions src/practice_stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ pub trait PracticeStats {
}

/// An implementation of [`PracticeStats`] backed by `SQLite`.
pub struct PracticeStatsDB {
pub struct LocalPracticeStats {
/// A pool of connections to the database.
pool: Pool<SqliteConnectionManager>,
}

impl PracticeStatsDB {
impl LocalPracticeStats {
/// Returns all the migrations needed to set up the database.
fn migrations() -> Migrations<'static> {
Migrations::new(vec![
Expand Down Expand Up @@ -98,16 +98,16 @@ impl PracticeStatsDB {
}

/// A constructor taking a `SQLite` connection manager.
fn new(connection_manager: SqliteConnectionManager) -> Result<PracticeStatsDB> {
fn new(connection_manager: SqliteConnectionManager) -> Result<LocalPracticeStats> {
// Create a connection pool and initialize the database.
let pool = Pool::new(connection_manager)?;
let mut stats = PracticeStatsDB { pool };
let mut stats = LocalPracticeStats { pool };
stats.init()?;
Ok(stats)
}

/// A constructor taking the path to a database file.
pub fn new_from_disk(db_path: &str) -> Result<PracticeStatsDB> {
pub fn new_from_disk(db_path: &str) -> Result<LocalPracticeStats> {
Self::new(db_utils::new_connection_manager(db_path))
}

Expand Down Expand Up @@ -213,7 +213,7 @@ impl PracticeStatsDB {
}
}

impl PracticeStats for PracticeStatsDB {
impl PracticeStats for LocalPracticeStats {
fn get_scores(
&self,
exercise_id: Ustr,
Expand Down Expand Up @@ -252,12 +252,12 @@ mod test {

use crate::{
data::{ExerciseTrial, MasteryScore},
practice_stats::{PracticeStats, PracticeStatsDB},
practice_stats::{LocalPracticeStats, PracticeStats},
};

fn new_tests_stats() -> Result<Box<dyn PracticeStats>> {
let connection_manager = SqliteConnectionManager::memory();
let practice_stats = PracticeStatsDB::new(connection_manager)?;
let practice_stats = LocalPracticeStats::new(connection_manager)?;
Ok(Box::new(practice_stats))
}

Expand Down
16 changes: 8 additions & 8 deletions src/review_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ pub trait ReviewList {
}

/// An implementation of [`ReviewList`] backed by `SQLite`.
pub struct ReviewListDB {
pub struct LocalReviewList {
/// A pool of connections to the database.
pool: Pool<SqliteConnectionManager>,
}

impl ReviewListDB {
impl LocalReviewList {
/// Returns all the migrations needed to set up the database.
fn migrations() -> Migrations<'static> {
Migrations::new(vec![
Expand All @@ -56,16 +56,16 @@ impl ReviewListDB {
}

/// A constructor taking a connection manager.
fn new(connection_manager: SqliteConnectionManager) -> Result<ReviewListDB> {
fn new(connection_manager: SqliteConnectionManager) -> Result<LocalReviewList> {
// Initialize the pool and the review list database.
let pool = Pool::new(connection_manager)?;
let mut review_list = ReviewListDB { pool };
let mut review_list = LocalReviewList { pool };
review_list.init()?;
Ok(review_list)
}

/// A constructor taking the path to the database file.
pub fn new_from_disk(db_path: &str) -> Result<ReviewListDB> {
pub fn new_from_disk(db_path: &str) -> Result<LocalReviewList> {
let connection_manager = SqliteConnectionManager::file(db_path).with_init(
|connection: &mut Connection| -> Result<(), rusqlite::Error> {
// The following pragma statements are set to improve the read and write performance
Expand Down Expand Up @@ -114,7 +114,7 @@ impl ReviewListDB {
}
}

impl ReviewList for ReviewListDB {
impl ReviewList for LocalReviewList {
fn add_to_review_list(&mut self, unit_id: Ustr) -> Result<(), ReviewListError> {
self.add_to_review_list_helper(unit_id)
.map_err(|e| ReviewListError::AddUnit(unit_id, e))
Expand All @@ -137,11 +137,11 @@ mod test {
use r2d2_sqlite::SqliteConnectionManager;
use ustr::Ustr;

use crate::review_list::{ReviewList, ReviewListDB};
use crate::review_list::{LocalReviewList, ReviewList};

fn new_test_review_list() -> Result<Box<dyn ReviewList>> {
let connection_manager = SqliteConnectionManager::memory();
let review_list = ReviewListDB::new(connection_manager)?;
let review_list = LocalReviewList::new(connection_manager)?;
Ok(Box::new(review_list))
}

Expand Down
3 changes: 0 additions & 3 deletions src/scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ use crate::{
ExerciseManifest, MasteryScore, SchedulerOptions, UnitType,
},
error::ExerciseSchedulerError,
graph::UnitGraph,
practice_stats::PracticeStats,
review_list::ReviewList,
scheduler::{cache::ScoreCache, data::SchedulerData, filter::CandidateFilter},
};

Expand Down
3 changes: 0 additions & 3 deletions src/scheduler/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ use std::cell::RefCell;
use ustr::{Ustr, UstrMap, UstrSet};

use crate::{
blacklist::Blacklist,
data::{SchedulerOptions, UnitType},
graph::UnitGraph,
practice_stats::PracticeStats,
scheduler::SchedulerData,
scorer::{ExerciseScorer, SimpleScorer},
};
Expand Down

0 comments on commit 4ae2992

Please sign in to comment.