From f1288e155e61425af702f78bc4509cff4ab37617 Mon Sep 17 00:00:00 2001 From: Alex Maldonado Date: Fri, 7 Aug 2026 10:43:42 -0400 Subject: [PATCH] refactor: splitting models --- Cargo.toml | 4 +- docs/guide/first_exam.md | 3 +- src/analysis/calibrate.rs | 16 +-- src/analysis/irt.rs | 31 ++--- src/analysis/students.rs | 2 +- src/authoring/jsonschema.rs | 10 +- src/authoring/lint.rs | 26 ++--- src/authoring/select.rs | 22 ++-- src/commands/analysis.rs | 10 +- src/commands/banks.rs | 9 +- src/commands/export.rs | 2 +- src/commands/project.rs | 11 +- src/data/canvas.rs | 19 ++-- src/data/gradescope.rs | 4 +- src/data/store_parquet.rs | 2 +- src/export/report.rs | 6 +- src/export/typst.rs | 6 +- src/export/typst/template.rs | 10 +- src/helpers.rs | 5 +- src/lib.rs | 8 +- src/main.rs | 2 +- src/model.rs | 2 + src/model/assessment.rs | 211 +---------------------------------- src/model/bank.rs | 18 +-- src/model/catalog.rs | 46 +++++++- src/model/course.rs | 97 +--------------- src/model/history.rs | 179 +++++++++++++++++++++++++++++ src/model/layout.rs | 99 ++++++++++++++++ src/util/hash.rs | 4 +- src/util/markup.rs | 2 +- 30 files changed, 453 insertions(+), 413 deletions(-) create mode 100644 src/model/history.rs create mode 100644 src/model/layout.rs diff --git a/Cargo.toml b/Cargo.toml index 4a12a79..5ad153d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "coursebank" version = "0.1.0" -edition = "2021" -rust-version = "1.75" +edition = "2024" +rust-version = "1.85" description = "Author, assemble, administer, and analyze leveled course assessments from YAML item banks" authors = ["Alex Maldonado "] license = "MIT" diff --git a/docs/guide/first_exam.md b/docs/guide/first_exam.md index 9325a33..ac42247 100644 --- a/docs/guide/first_exam.md +++ b/docs/guide/first_exam.md @@ -226,7 +226,8 @@ Assembling a form programmatically: use std::collections::BTreeMap; use std::path::Path; -use coursebank::assessment::{Blueprint, History}; +use coursebank::assessment::Blueprint; +use coursebank::history::History; use coursebank::date::Date; use coursebank::{select, Catalog, Level}; diff --git a/src/analysis/calibrate.rs b/src/analysis/calibrate.rs index 8069402..a89b6bc 100644 --- a/src/analysis/calibrate.rs +++ b/src/analysis/calibrate.rs @@ -622,7 +622,7 @@ pub fn apply(plan: &Plan) -> Result> { "{} — the bank changed since the plan was built; re-run calibration", path.display() )), - }) + }); } } } @@ -794,12 +794,14 @@ mod tests { next.flags = vec![Flag::NegativeDiscrimination]; let diff = diff_calibration(Some(&previous), &next); - assert!(diff - .iter() - .any(|d| d.contains("flags added") && d.contains("negative_discrimination"))); - assert!(diff - .iter() - .any(|d| d.contains("flags cleared") && d.contains("too_easy"))); + assert!( + diff.iter() + .any(|d| d.contains("flags added") && d.contains("negative_discrimination")) + ); + assert!( + diff.iter() + .any(|d| d.contains("flags cleared") && d.contains("too_easy")) + ); } #[test] diff --git a/src/analysis/irt.rs b/src/analysis/irt.rs index dc7d394..119946c 100644 --- a/src/analysis/irt.rs +++ b/src/analysis/irt.rs @@ -197,11 +197,7 @@ impl ItemFit { // For the 3PL this reduces to the 2PL form when c = 0. let num = self.a * self.a * (p - c) * (p - c) * (1.0 - p); let den = (1.0 - c) * (1.0 - c) * p; - if den <= 0.0 { - 0.0 - } else { - num / den - } + if den <= 0.0 { 0.0 } else { num / den } } } @@ -424,7 +420,7 @@ pub fn fit(matrix: &Matrix, opts: &Options) -> Fit { let mut a = vec![1.0f64; j_count]; let mut b = vec![0.0f64; j_count]; let mut c = vec![0.0f64; j_count]; - for j in 0..j_count { + for (j, b_j) in b.iter_mut().enumerate() { let column = matrix.column(j); let answered: Vec = column.into_iter().flatten().collect(); if answered.is_empty() { @@ -434,7 +430,7 @@ pub fn fit(matrix: &Matrix, opts: &Options) -> Fit { let clamped = p.clamp(0.03, 0.97); // Inverse logistic of the p-value, which is the difficulty a Rasch model // implies when discrimination is one. - b[j] = -(clamped / (1.0 - clamped)).ln(); + *b_j = -(clamped / (1.0 - clamped)).ln(); } let mut iterations = 0usize; @@ -1102,8 +1098,10 @@ mod tests { #[test] fn rasch_fixes_discrimination_at_one() { - let mut opts = Options::default(); - opts.model = IrtModel::Rasch; + let opts = Options { + model: IrtModel::Rasch, + ..Default::default() + }; let f = fit(&ordered_matrix(), &opts); assert!(f.items.iter().all(|i| (i.a - 1.0).abs() < 1e-12)); assert!(f.items.iter().all(|i| i.c.is_none())); @@ -1111,14 +1109,17 @@ mod tests { #[test] fn three_pl_reports_a_lower_asymptote_and_warns() { - let mut opts = Options::default(); - opts.model = IrtModel::ThreePl; + let opts = Options { + model: IrtModel::ThreePl, + ..Default::default() + }; let f = fit(&ordered_matrix(), &opts); assert!(f.items.iter().all(|i| i.c.is_some())); - assert!(f - .items - .iter() - .all(|i| i.c.unwrap() >= 0.0 && i.c.unwrap() <= 0.4)); + assert!( + f.items + .iter() + .all(|i| i.c.unwrap() >= 0.0 && i.c.unwrap() <= 0.4) + ); assert!(f.warnings.iter().any(|w| w.contains("1000"))); } diff --git a/src/analysis/students.rs b/src/analysis/students.rs index f6f0141..165ad92 100644 --- a/src/analysis/students.rs +++ b/src/analysis/students.rs @@ -883,7 +883,7 @@ pub fn cluster(students: &[StudentSummary], k: usize) -> Vec { level_means, }); } - out.sort_by(|a, b| b.members.len().cmp(&a.members.len())); + out.sort_by_key(|b| std::cmp::Reverse(b.members.len())); out } diff --git a/src/authoring/jsonschema.rs b/src/authoring/jsonschema.rs index 940d0c8..c615220 100644 --- a/src/authoring/jsonschema.rs +++ b/src/authoring/jsonschema.rs @@ -13,7 +13,7 @@ //! generic derivation would emit as bare strings. The crate's own validation //! remains authoritative; the schema is for the editor. -use serde_json::{json, Value}; +use serde_json::{Value, json}; use crate::course::SCHEMA_VERSION; use crate::error::Result; @@ -998,9 +998,11 @@ mod tests { "# yaml-language-server: $schema=../.coursebank/schema/bank.schema.json" ); // A trailing slash must not double up. - assert!(Kind::Course - .modeline("schema/") - .ends_with("schema/course.schema.json")); + assert!( + Kind::Course + .modeline("schema/") + .ends_with("schema/course.schema.json") + ); } #[test] diff --git a/src/authoring/lint.rs b/src/authoring/lint.rs index a2dd17c..28c566b 100644 --- a/src/authoring/lint.rs +++ b/src/authoring/lint.rs @@ -1058,11 +1058,7 @@ fn jaccard(a: &BTreeSet, b: &BTreeSet) -> f64 { } let inter = a.intersection(b).count() as f64; let union = a.union(b).count() as f64; - if union == 0.0 { - 0.0 - } else { - inter / union - } + if union == 0.0 { 0.0 } else { inter / union } } /// The Flesch-Kincaid grade level of a passage. @@ -1313,8 +1309,9 @@ options: #[test] fn stem_without_a_task_is_flagged_but_completions_are_not() { - assert!(codes( - r#" + assert!( + codes( + r#" id: q-a-001 status: draft level: 1 @@ -1323,11 +1320,13 @@ options: - { id: A, text: aaaa, correct: true } - { id: B, text: bbbb } "# - ) - .contains(&"clarity-no-task")); + ) + .contains(&"clarity-no-task") + ); - assert!(!codes( - r#" + assert!( + !codes( + r#" id: q-a-001 status: draft level: 1 @@ -1336,8 +1335,9 @@ options: - { id: A, text: aaaa, correct: true } - { id: B, text: bbbb } "# - ) - .contains(&"clarity-no-task")); + ) + .contains(&"clarity-no-task") + ); } #[test] diff --git a/src/authoring/select.rs b/src/authoring/select.rs index 6e4be18..41a1574 100644 --- a/src/authoring/select.rs +++ b/src/authoring/select.rs @@ -23,13 +23,12 @@ use std::collections::{BTreeMap, BTreeSet}; -use crate::assessment::{ - Assessment, AssessmentFile, Blueprint, Form, History, Kind, Placement, Platform, -}; +use crate::assessment::{Assessment, AssessmentFile, Blueprint, Form, Kind, Placement, Platform}; use crate::catalog::Catalog; use crate::course::SCHEMA_VERSION; use crate::date::Date; use crate::error::{Error, Result}; +use crate::history::History; use crate::rng::Rng; use crate::taxonomy::Level; @@ -444,14 +443,14 @@ pub fn to_record( ) -> Result { let default_points = catalog.course.policy.points_per_item; let mut items = Vec::new(); - let mut number = 1u32; - for (uid, is_bonus) in selection - .scored - .iter() - .map(|u| (u, false)) - .chain(selection.bonus.iter().map(|u| (u, true))) - { + for (number, (uid, is_bonus)) in (1u32..).zip( + selection + .scored + .iter() + .map(|u| (u, false)) + .chain(selection.bonus.iter().map(|u| (u, true))), + ) { let e = catalog.require(uid)?; items.push(Placement { number, @@ -466,7 +465,6 @@ pub fn to_record( credit_overrides: BTreeMap::new(), dropped: false, }); - number += 1; } let form_list: Vec
= (0..forms) @@ -553,7 +551,7 @@ pub fn layout(record: &AssessmentFile, form: &Form) -> Vec { rng.shuffle(&mut scored); } - scored.into_iter().chain(bonus.into_iter()).collect() + scored.into_iter().chain(bonus).collect() } /// The option order for one item on one form. diff --git a/src/commands/analysis.rs b/src/commands/analysis.rs index a23d1aa..4b49bbc 100644 --- a/src/commands/analysis.rs +++ b/src/commands/analysis.rs @@ -8,10 +8,10 @@ use coursebank::calibrate; use coursebank::canvas; use coursebank::classical::{self, Thresholds}; -use coursebank::course::Layout; use coursebank::error::Result; use coursebank::gradescope; use coursebank::irt; +use coursebank::layout::Layout; use coursebank::report; use coursebank::store::{self, Store}; use coursebank::students; @@ -114,8 +114,8 @@ pub(crate) fn analyze(cli: &Cli, sub: &AnalyzeCommand) -> Result { } println!("{}\n", analysis.reliability.interpretation()); println!( - "{:>3} {:>5} {:>6} {:>6} {:>6} {}", - "Q", "p", "r", "D", "blank", "FLAGS" + "{:>3} {:>5} {:>6} {:>6} {:>6} FLAGS", + "Q", "p", "r", "D", "blank" ); for item in &analysis.items { println!( @@ -184,8 +184,8 @@ pub(crate) fn analyze(cli: &Cli, sub: &AnalyzeCommand) -> Result { ); println!( - "{:>3} {:>6} {:>7} {:>7} {:>7} {}", - "Q", "a", "b", "SE(a)", "SE(b)", "NOTES" + "{:>3} {:>6} {:>7} {:>7} {:>7} NOTES", + "Q", "a", "b", "SE(a)", "SE(b)" ); for item in &fit.items { println!( diff --git a/src/commands/banks.rs b/src/commands/banks.rs index 0499b50..4c73160 100644 --- a/src/commands/banks.rs +++ b/src/commands/banks.rs @@ -6,11 +6,12 @@ use std::collections::BTreeMap; -use coursebank::assessment::{AssessmentFile, Blueprint, History}; +use coursebank::assessment::{AssessmentFile, Blueprint}; use coursebank::bank::BankFile; -use coursebank::course::Layout; use coursebank::date::Date; use coursebank::error::{Error, Result}; +use coursebank::history::History; +use coursebank::layout::Layout; use coursebank::select; use coursebank::yaml; @@ -102,7 +103,7 @@ pub(crate) fn assessment(cli: &Cli, sub: &AssessmentCommand) -> Result let catalog = load(cli)?; let record = load_record(&catalog, id)?; print_record(&catalog, &record); - let issues = record.validate(Some(&catalog)); + let issues = catalog.validate_record(&record); if !issues.is_empty() { println!("\n{} problem(s):", issues.len()); for issue in &issues { @@ -213,7 +214,7 @@ pub(crate) fn usage(cli: &Cli, sub: &UsageCommand) -> Result { Some(id) => vec![catalog.resolve(id)?], None => catalog.entries.iter().map(|e| e.uid.clone()).collect(), }; - println!("{:<34} {:>5} {:<12} {}", "ITEM", "USES", "LAST", "WHERE"); + println!("{:<34} {:>5} {:<12} WHERE", "ITEM", "USES", "LAST"); for uid in uids { let uses = history.for_item(&uid); if uses.is_empty() && item.is_none() { diff --git a/src/commands/export.rs b/src/commands/export.rs index 28b1259..765ebdf 100644 --- a/src/commands/export.rs +++ b/src/commands/export.rs @@ -6,8 +6,8 @@ //! [`pick_variants`], which resolves the `--variant` flags into a canonical list. use coursebank::assessment::Form; -use coursebank::course::Layout; use coursebank::error::{Error, Result}; +use coursebank::layout::Layout; use coursebank::qti; use coursebank::typst; use coursebank::yaml; diff --git a/src/commands/project.rs b/src/commands/project.rs index dcf723c..a046353 100644 --- a/src/commands/project.rs +++ b/src/commands/project.rs @@ -9,9 +9,10 @@ use std::collections::BTreeMap; use coursebank::assessment::AssessmentFile; use coursebank::bank::BankFile; -use coursebank::course::{CourseFile, Layout, COURSE_FILE}; +use coursebank::course::{COURSE_FILE, CourseFile}; use coursebank::error::{Error, Result}; use coursebank::jsonschema; +use coursebank::layout::Layout; use coursebank::lint::{self, Rule}; use coursebank::taxonomy::Level; use coursebank::yaml; @@ -98,7 +99,7 @@ pub(crate) fn validate(cli: &Cli) -> Result { let records = AssessmentFile::load_all(&catalog.layout.assessments())?; let mut all = issues; for record in &records { - for issue in record.validate(Some(&catalog)) { + for issue in catalog.validate_record(record) { all.push(format!("{}: {issue}", record.assessment.id)); } } @@ -128,7 +129,7 @@ pub(crate) fn validate(cli: &Cli) -> Result { /// [`Outcome::Findings`] when anything fires, unless `--no-fail` was given. pub(crate) fn lint(cli: &Cli, args: &LintArgs) -> Result { if args.list_rules { - println!("{:<32} {:<8} {}", "CODE", "SEVERITY", "WHAT IT CATCHES"); + println!("{:<32} {:<8} WHAT IT CATCHES", "CODE", "SEVERITY"); for rule in Rule::ALL { println!( "{:<32} {:<8} {}", @@ -230,8 +231,8 @@ pub(crate) fn catalog(cli: &Cli, args: &CatalogArgs) -> Result { let coverage = catalog.coverage(); println!("\nObjective coverage:"); println!( - " {:<40} {:>6} {:>6} {}", - "OBJECTIVE", "ITEMS", "READY", "MAX LEVEL" + " {:<40} {:>6} {:>6} MAX LEVEL", + "OBJECTIVE", "ITEMS", "READY" ); for row in &coverage.rows { println!( diff --git a/src/data/canvas.rs b/src/data/canvas.rs index 0bacc19..030cd72 100644 --- a/src/data/canvas.rs +++ b/src/data/canvas.rs @@ -26,7 +26,7 @@ use crate::assessment::AssessmentFile; use crate::catalog::Catalog; use crate::date::Date; use crate::error::{Error, Result}; -use crate::responses::{administration_id, Response, ResponseSet}; +use crate::responses::{Response, ResponseSet, administration_id}; /// What the ingest needs that the export does not carry. pub type Context = crate::gradescope::Context; @@ -120,11 +120,9 @@ pub fn normalize(s: &str) -> String { if c.is_alphanumeric() { out.push(c); last_space = false; - } else if c.is_whitespace() || c == '-' || c == '_' { - if !last_space { - out.push(' '); - last_space = true; - } + } else if (c.is_whitespace() || c == '-' || c == '_') && !last_space { + out.push(' '); + last_space = true; } // Everything else — punctuation, entity leftovers — is dropped. } @@ -579,10 +577,11 @@ mod tests { // Two real students, two questions each. The preview row is discarded. assert_eq!(set.rows.len(), 4, "{:?}", set.warnings); - assert!(set - .rows - .iter() - .all(|r| r.name.as_deref() != Some("Test Student"))); + assert!( + set.rows + .iter() + .all(|r| r.name.as_deref() != Some("Test Student")) + ); assert_eq!(set.scored_total("1234567"), 2.0); assert_eq!(set.scored_total("7654321"), 1.0); diff --git a/src/data/gradescope.rs b/src/data/gradescope.rs index 0b482f3..7352825 100644 --- a/src/data/gradescope.rs +++ b/src/data/gradescope.rs @@ -45,7 +45,7 @@ use std::path::{Path, PathBuf}; use crate::date::Date; use crate::error::{Error, Result}; -use crate::responses::{administration_id, Response, ResponseSet}; +use crate::responses::{Response, ResponseSet, administration_id}; use crate::taxonomy::Flag; /// What a rubric column represents. @@ -358,7 +358,7 @@ pub fn parse_question(path: &Path) -> Result { "{} has no `Submission Time` column, so the rubric columns cannot be located; \ is this a Gradescope per-question export?", path.display() - )])) + )])); } }; let end = index_of("Adjustment").unwrap_or(header.len()); diff --git a/src/data/store_parquet.rs b/src/data/store_parquet.rs index 5a6f9a7..1040973 100644 --- a/src/data/store_parquet.rs +++ b/src/data/store_parquet.rs @@ -17,8 +17,8 @@ use std::sync::Arc; use arrow_array::{ArrayRef, BooleanArray, Float64Array, RecordBatch, StringArray, UInt32Array}; use arrow_schema::{DataType, Field, Schema}; -use parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder; use parquet::arrow::ArrowWriter; +use parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder; use parquet::basic::Compression; use parquet::file::properties::WriterProperties; diff --git a/src/export/report.rs b/src/export/report.rs index 4b859e0..0204fdb 100644 --- a/src/export/report.rs +++ b/src/export/report.rs @@ -145,11 +145,7 @@ pub fn student( out.push_str("## What this exam says about each learning objective\n\n"); out.push_str("| | Objective | You | Class | Items |\n|:--|:--|--:|--:|--:|\n"); for o in &summary.objectives { - let you = if o.status == Mastery::NotEnoughEvidence { - format!("{:.0}%", o.rate * 100.0) - } else { - format!("{:.0}%", o.rate * 100.0) - }; + let you = format!("{:.0}%", o.rate * 100.0); out.push_str(&format!( "| {} | {} | {} | {:.0}% | {} |\n", o.status.symbol(), diff --git a/src/export/typst.rs b/src/export/typst.rs index 6503b0e..1820155 100644 --- a/src/export/typst.rs +++ b/src/export/typst.rs @@ -53,16 +53,16 @@ pub mod value; use std::path::{Path, PathBuf}; pub use config::{ - ConfigFile, ContentMode, Fields, LetterStyle, Overrides, RenderConfig, Reveal, StimulusMode, - Variant, CONFIG_FILE, CONFIG_TEMPLATE, + CONFIG_FILE, CONFIG_TEMPLATE, ConfigFile, ContentMode, Fields, LetterStyle, Overrides, + RenderConfig, Reveal, StimulusMode, Variant, }; pub use payload::Payload; pub use template::{Origin, Slot, Template}; pub use value::Value; +use crate::Layout; use crate::assessment::{AssessmentFile, Form}; use crate::catalog::Catalog; -use crate::course::Layout; use crate::error::{Error, Result}; /// What to render. diff --git a/src/export/typst/template.rs b/src/export/typst/template.rs index e3eedea..c4b4264 100644 --- a/src/export/typst/template.rs +++ b/src/export/typst/template.rs @@ -55,7 +55,7 @@ use std::path::{Path, PathBuf}; -use crate::course::Layout; +use crate::Layout; use crate::error::{Error, Result}; use super::config::Variant; @@ -702,9 +702,11 @@ mod tests { #[test] fn ordinary_comments_are_left_alone() { - assert!(parse("// nothing to see\n// coursebank\n") - .unwrap() - .is_inert()); + assert!( + parse("// nothing to see\n// coursebank\n") + .unwrap() + .is_inert() + ); // A word that merely starts with `begin` is a slot name, not a keyword. let err = parse("// coursebank:beginning\n").unwrap_err(); assert!(err.to_string().contains("unknown slot `beginning`")); diff --git a/src/helpers.rs b/src/helpers.rs index aa5512c..ea496d8 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -14,10 +14,11 @@ use std::path::Path; use coursebank::assessment::{AssessmentFile, Form}; use coursebank::catalog::Catalog; -use coursebank::course::{Layout, COURSE_FILE}; +use coursebank::course::COURSE_FILE; use coursebank::date::Date; use coursebank::error::{Error, Result}; use coursebank::gradescope; +use coursebank::layout::Layout; use coursebank::responses::ResponseSet; use coursebank::store::Store; use coursebank::taxonomy::Level; @@ -223,7 +224,7 @@ pub(crate) fn print_record(catalog: &Catalog, record: &AssessmentFile) { ); println!( "estimated {:.0} minutes of working time", - record.estimated_minutes(catalog) + catalog.estimated_minutes(record) ); println!("\nBy level:"); diff --git a/src/lib.rs b/src/lib.rs index 62f4b80..b78913a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -44,7 +44,7 @@ //! //! Assessment records are the single source of truth for reuse history. There //! is no separate ledger file, because a ledger duplicates what the records must -//! already get right and then drifts from it. [`assessment::History`] derives usage +//! already get right and then drifts from it. [`history::History`] derives usage //! by scanning the records. //! //! Fingerprints cover only what a student saw. Retag an item's metadata and its @@ -96,7 +96,7 @@ pub mod util; pub use util::{date, hash, markup, rng, yaml, zipfile}; -pub use model::{assessment, bank, catalog, course, item, taxonomy}; +pub use model::{assessment, bank, catalog, course, history, item, layout, taxonomy}; pub use authoring::{jsonschema, lint, select}; @@ -109,9 +109,11 @@ pub use analysis::{calibrate, classical, irt, students}; pub use export::{qti, report, typst}; pub use catalog::Catalog; -pub use course::{CourseFile, Layout, SCHEMA_VERSION}; +pub use course::{CourseFile, SCHEMA_VERSION}; pub use error::{Error, Result}; +pub use history::History; pub use item::Item; +pub use layout::Layout; pub use taxonomy::{CognitiveProcess, ErrorType, Flag, Format, Level, Status}; /// Version of package. diff --git a/src/main.rs b/src/main.rs index f8b9974..56fd99a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,7 +33,7 @@ use std::process::ExitCode; use clap::Parser; use crate::cli::Cli; -use crate::commands::{run, Outcome}; +use crate::commands::{Outcome, run}; /// Parses the command line, runs the requested command, and maps its result onto /// a process exit code. diff --git a/src/model.rs b/src/model.rs index 6e81d72..ddf8505 100644 --- a/src/model.rs +++ b/src/model.rs @@ -24,5 +24,7 @@ pub mod assessment; pub mod bank; pub mod catalog; pub mod course; +pub mod history; pub mod item; +pub mod layout; pub mod taxonomy; diff --git a/src/model/assessment.rs b/src/model/assessment.rs index 228facd..c121d8e 100644 --- a/src/model/assessment.rs +++ b/src/model/assessment.rs @@ -24,7 +24,6 @@ use std::path::Path; use serde::{Deserialize, Serialize}; -use crate::catalog::Catalog; use crate::course::SCHEMA_VERSION; use crate::date::Date; use crate::error::Result; @@ -396,7 +395,7 @@ impl AssessmentFile { /// # Returns /// /// Every problem found. - pub fn validate(&self, catalog: Option<&Catalog>) -> Vec { + pub fn validate(&self) -> Vec { let mut issues = Vec::new(); if self.assessment.id.trim().is_empty() { @@ -458,60 +457,9 @@ impl AssessmentFile { form_ids.push(&form.id); } - if let Some(cat) = catalog { - for p in &self.items { - match cat.get(&p.item) { - None => { - issues.push(format!("question {}: unknown item `{}`", p.number, p.item)) - } - Some(entry) => { - if let Some(fp) = &p.fingerprint { - if *fp != entry.item.fingerprint() { - issues.push(format!( - "question {} ({}): the item has been edited since this \ - assessment; statistics from this administration describe \ - the older wording", - p.number, p.item - )); - } - } - if !p.key.is_empty() && p.key != entry.item.key_letters() { - issues.push(format!( - "question {} ({}): the recorded key {:?} differs from the \ - item's current key {:?}", - p.number, - p.item, - p.key, - entry.item.key_letters() - )); - } - } - } - } - } - issues } - /// Estimated time to complete, in minutes. - /// - /// # Arguments - /// - /// * `catalog` - the loaded course, for per-item time estimates. - /// - /// # Returns - /// - /// The estimate. - pub fn estimated_minutes(&self, catalog: &Catalog) -> f64 { - let seconds: f64 = self - .items - .iter() - .filter_map(|p| catalog.get(&p.item)) - .map(|e| e.item.expected_seconds()) - .sum(); - seconds / 60.0 - } - /// A skeleton record for `coursebank assessment new`. /// /// # Arguments @@ -566,113 +514,6 @@ pub struct Usage { pub fingerprint: Option, } -/// The usage history of a course, built by scanning assessment records. -/// -/// There is deliberately no separate ledger file. A ledger duplicates -/// information that the records already hold and then drifts away from it; this -/// derives the same answers from the one artifact that has to be right anyway. -#[derive(Debug, Clone, Default)] -pub struct History { - /// Every recorded appearance, newest last. - pub usages: Vec, -} - -impl History { - /// Builds the history from a directory of assessment records. - /// - /// # Arguments - /// - /// * `dir` - the assessments directory. - /// - /// # Returns - /// - /// The history. - /// - /// # Errors - /// - /// Propagates load errors. - pub fn load(dir: &Path) -> Result { - let mut usages = Vec::new(); - for file in AssessmentFile::load_all(dir)? { - for p in &file.items { - usages.push(Usage { - item: p.item.clone(), - assessment: file.assessment.id.clone(), - title: file.assessment.title.clone(), - kind: file.assessment.kind, - date: file.assessment.date, - number: p.number, - fingerprint: p.fingerprint.clone(), - }); - } - } - usages.sort_by(|a, b| { - a.date - .cmp(&b.date) - .then(a.assessment.cmp(&b.assessment)) - .then(a.number.cmp(&b.number)) - }); - Ok(History { usages }) - } - - /// Every appearance of one item, oldest first. - /// - /// # Arguments - /// - /// * `uid` - the item's global id. - /// - /// # Returns - /// - /// Matching usages. - pub fn for_item(&self, uid: &str) -> Vec<&Usage> { - self.usages.iter().filter(|u| u.item == uid).collect() - } - - /// The most recent date an item was used. - /// - /// # Arguments - /// - /// * `uid` - the item's global id. - /// - /// # Returns - /// - /// The date, or `None` when never used or never dated. - pub fn last_used(&self, uid: &str) -> Option { - self.for_item(uid).iter().filter_map(|u| u.date).max() - } - - /// How many times an item has been used. - /// - /// # Arguments - /// - /// * `uid` - the item's global id. - /// - /// # Returns - /// - /// The count. - pub fn use_count(&self, uid: &str) -> usize { - self.for_item(uid).len() - } - - /// Whether an item is still inside its reuse cooldown. - /// - /// # Arguments - /// - /// * `uid` - the item's global id. - /// * `cooldown_days` - the minimum gap between uses. - /// * `as_of` - the date of the assessment being assembled. - /// - /// # Returns - /// - /// `true` when the item was used too recently to reuse. - pub fn in_cooldown(&self, uid: &str, cooldown_days: i64, as_of: Date) -> bool { - match self.last_used(uid) { - Some(last) => last.days_until(as_of) < cooldown_days, - None => false, - } - } -} - fn default_version() -> String { SCHEMA_VERSION.to_string() } @@ -716,7 +557,7 @@ items: assert_eq!(a.items.len(), 3); assert_eq!(a.total_points(1.0), 3.0, "bonus is excluded"); assert_eq!(a.placement(2).unwrap().item, "b1::q-a-002"); - assert!(a.validate(None).is_empty(), "{:?}", a.validate(None)); + assert!(a.validate().is_empty(), "{:?}", a.validate()); } #[test] @@ -737,7 +578,7 @@ items: - { number: 1, item: "b::q-1" } "#, ); - let issues = a.validate(None); + let issues = a.validate(); assert!(issues.iter().any(|i| i.contains("used 2 times"))); assert!(issues.iter().any(|i| i.contains("appears twice"))); } @@ -752,10 +593,7 @@ items: - { number: 3, item: "b::q-2" } "#, ); - assert!(a - .validate(None) - .iter() - .any(|i| i.contains("not contiguous"))); + assert!(a.validate().iter().any(|i| i.contains("not contiguous"))); } #[test] @@ -767,46 +605,7 @@ items: - { number: 1, item: "b::q-1", credit_overrides: { B: 1.5 } } "#, ); - assert!(a - .validate(None) - .iter() - .any(|i| i.contains("must be in [0, 1]"))); - } - - #[test] - fn history_tracks_last_use_and_cooldown() { - let h = History { - usages: vec![ - Usage { - item: "b::q-1".into(), - assessment: "exam-1".into(), - title: "Exam 1".into(), - kind: Kind::Exam, - date: Some("2025-09-15".parse().unwrap()), - number: 4, - fingerprint: None, - }, - Usage { - item: "b::q-1".into(), - assessment: "exam-3".into(), - title: "Exam 3".into(), - kind: Kind::Exam, - date: Some("2026-02-10".parse().unwrap()), - number: 7, - fingerprint: None, - }, - ], - }; - assert_eq!(h.use_count("b::q-1"), 2); - assert_eq!(h.use_count("b::q-2"), 0); - assert_eq!(h.last_used("b::q-1").unwrap().to_string(), "2026-02-10"); - - let exam_date: Date = "2026-04-23".parse().unwrap(); - // 72 days elapsed, so a 90-day cooldown still blocks it and a 60-day one - // does not. - assert!(h.in_cooldown("b::q-1", 90, exam_date)); - assert!(!h.in_cooldown("b::q-1", 60, exam_date)); - assert!(!h.in_cooldown("b::never-used", 3650, exam_date)); + assert!(a.validate().iter().any(|i| i.contains("must be in [0, 1]"))); } #[test] diff --git a/src/model/bank.rs b/src/model/bank.rs index c6b11a2..0231ad1 100644 --- a/src/model/bank.rs +++ b/src/model/bank.rs @@ -491,7 +491,7 @@ fn validate_item( )); } } - for (letter, _) in &c.option_stats { + for letter in c.option_stats.keys() { if it.option(letter).is_none() { issues.push(format!( "calibration.option_stats has `{letter}`, which is not an option of this item" @@ -662,9 +662,11 @@ mod tests { "#, ); let issues = b.validate(None); - assert!(issues - .iter() - .any(|i| i.contains("exactly one keyed option"))); + assert!( + issues + .iter() + .any(|i| i.contains("exactly one keyed option")) + ); assert_eq!( issues .iter() @@ -831,9 +833,11 @@ learning_objectives: "#, ); let issues = b.validate(Some(&course)); - assert!(issues - .iter() - .any(|i| i.contains("unknown learning objective `lo-unknown`"))); + assert!( + issues + .iter() + .any(|i| i.contains("unknown learning objective `lo-unknown`")) + ); assert!(issues.iter().any(|i| i.contains("unknown lecture `L99`"))); assert!( issues.iter().any(|i| i.contains("exceeds the ceiling")), diff --git a/src/model/catalog.rs b/src/model/catalog.rs index 349d2e3..b8f9fe3 100644 --- a/src/model/catalog.rs +++ b/src/model/catalog.rs @@ -16,10 +16,12 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; +use crate::assessment::AssessmentFile; use crate::bank::BankFile; -use crate::course::{CourseFile, Layout}; +use crate::course::CourseFile; use crate::error::{Error, Result}; use crate::item::Item; +use crate::layout::Layout; use crate::taxonomy::{Level, Status}; use crate::yaml; @@ -249,6 +251,46 @@ impl Catalog { Ok(issues) } + /// Validates a record's internal invariants, then its references against this + /// catalog: unknown items, keys that drifted, and fingerprints showing the + /// item was reworded since it was administered. + pub fn validate_record(&self, record: &AssessmentFile) -> Vec { + let mut issues = record.validate(); + for p in &record.items { + match self.get(&p.item) { + None => issues.push(format!("question {}: unknown item `{}`", p.number, p.item)), + Some(entry) => { + if let Some(fp) = &p.fingerprint { + if *fp != entry.item.fingerprint() { + issues.push(format!( + "question {} ({}): the item has been edited since this assessment; \ + statistics from this administration describe the older wording", + p.number, p.item + )); + } + } + if !p.key.is_empty() && p.key != entry.item.key_letters() { + issues.push(format!( + "question {} ({}): the recorded key {:?} differs from the item's current key {:?}", + p.number, p.item, p.key, entry.item.key_letters())); + } + } + } + } + issues + } + + /// Estimated working time for a record, in minutes. + pub fn estimated_minutes(&self, record: &AssessmentFile) -> f64 { + let seconds: f64 = record + .items + .iter() + .filter_map(|p| self.get(&p.item)) + .map(|e| e.item.expected_seconds()) + .sum(); + seconds / 60.0 + } + /// Counts of assemblable, non-bonus items by level. /// /// # Returns @@ -391,7 +433,7 @@ impl Catalog { } } - for (lec_id, _) in &self.course.lectures { + for lec_id in self.course.lectures.keys() { if self .by_lecture(lec_id) .iter() diff --git a/src/model/course.rs b/src/model/course.rs index 6ed7d29..06132f5 100644 --- a/src/model/course.rs +++ b/src/model/course.rs @@ -12,7 +12,7 @@ //! belongs to the course and the administration, never to the item. use std::collections::BTreeMap; -use std::path::{Path, PathBuf}; +use std::path::Path; use serde::{Deserialize, Serialize}; @@ -307,7 +307,7 @@ impl CourseFile { issues.push(format!("lecture `{id}`: empty title")); } if let Some(u) = &lec.unit { - if !unit_ids.iter().any(|x| *x == u) { + if !unit_ids.contains(&u) { issues.push(format!("lecture `{id}`: unknown unit `{u}`")); } } @@ -318,7 +318,7 @@ impl CourseFile { issues.push(format!("objective `{id}`: empty text")); } if let Some(u) = &lo.unit { - if !unit_ids.iter().any(|x| *x == u) { + if !unit_ids.contains(&u) { issues.push(format!("objective `{id}`: unknown unit `{u}`")); } } @@ -550,97 +550,6 @@ impl CourseFile { } } -/// The standard directory layout of a course, resolved from a root. -/// -/// Keeping the layout in one type means every command agrees on where things -/// live, and a future rearrangement touches this struct only. -#[derive(Debug, Clone)] -pub struct Layout { - /// The course root. - pub root: PathBuf, -} - -impl Layout { - /// Builds a layout from a course root directory. - /// - /// # Arguments - /// - /// * `root` - the course directory. - /// - /// # Returns - /// - /// The layout. - pub fn new(root: impl Into) -> Layout { - Layout { root: root.into() } - } - - /// Path to `course.yaml`. - pub fn course_file(&self) -> PathBuf { - self.root.join(COURSE_FILE) - } - - /// Directory holding item bank YAML files. - pub fn banks(&self) -> PathBuf { - self.root.join("banks") - } - - /// Directory holding assessment records. - pub fn assessments(&self) -> PathBuf { - self.root.join("assessments") - } - - /// Directory holding response tables and derived statistics. - pub fn data(&self) -> PathBuf { - self.root.join("data") - } - - /// Directory holding generated reports. - pub fn reports(&self) -> PathBuf { - self.root.join("reports") - } - - /// Directory holding generated exports such as QTI packages. - pub fn build(&self) -> PathBuf { - self.root.join("build") - } - - /// Directory holding emitted JSON Schema files for editor validation. - pub fn schema(&self) -> PathBuf { - self.root.join("schema") - } - - /// Directory holding Typst export templates and their configuration. - /// - /// Unlike the other directories, this one is *not* created by - /// [`Layout::create_all`]. Its absence is meaningful: a course with no - /// `templates/` directory uses the templates compiled into the binary, and - /// creating an empty one on `init` would suggest a customization step is - /// required when it is not. `coursebank template dump` creates it on demand. - pub fn templates(&self) -> PathBuf { - self.root.join("templates") - } - - /// Creates every directory in the layout. - /// - /// # Errors - /// - /// Returns [`Error::Io`] if a directory cannot be created. - pub fn create_all(&self) -> Result<()> { - for dir in [ - self.root.clone(), - self.banks(), - self.assessments(), - self.data(), - self.reports(), - self.build(), - self.schema(), - ] { - std::fs::create_dir_all(&dir).map_err(|e| Error::io(&dir, e))?; - } - Ok(()) - } -} - /// Lowercases and hyphenates a string for use in file names and ids. /// /// # Arguments diff --git a/src/model/history.rs b/src/model/history.rs new file mode 100644 index 0000000..b4cefd3 --- /dev/null +++ b/src/model/history.rs @@ -0,0 +1,179 @@ +//! The usage history of a course, derived by scanning assessment records. +//! +//! There is deliberately no separate ledger file. A ledger duplicates +//! information the records already hold and then drifts away from it; this +//! derives the same answers from the one artifact that has to be right anyway. +//! An item was used exactly when it appears on a record. +//! +//! This is an aggregate over the records, the same kind of thing as +//! [`crate::catalog::Catalog`], which is why it sits beside the schema rather +//! than inside [`crate::assessment`]. + +use std::path::Path; + +use crate::assessment::{AssessmentFile, Kind}; +use crate::date::Date; +use crate::error::Result; + +/// One recorded appearance of an item, derived from the assessment records. +#[derive(Debug, Clone)] +pub struct Usage { + /// The item's global id. + pub item: String, + /// The assessment id it appeared on. + pub assessment: String, + /// The assessment title. + pub title: String, + /// The kind of assessment. + pub kind: Kind, + /// When it was administered, when recorded. + pub date: Option, + /// The printed question number. + pub number: u32, + /// The fingerprint as used. + pub fingerprint: Option, +} + +/// The usage history of a course, built by scanning assessment records. +#[derive(Debug, Clone, Default)] +pub struct History { + /// Every recorded appearance, newest last. + pub usages: Vec, +} + +impl History { + /// Builds the history from a directory of assessment records. + /// + /// # Arguments + /// + /// * `dir` - the assessments directory. + /// + /// # Returns + /// + /// The history. + /// + /// # Errors + /// + /// Propagates load errors. + pub fn load(dir: &Path) -> Result { + let mut usages = Vec::new(); + for file in AssessmentFile::load_all(dir)? { + for p in &file.items { + usages.push(Usage { + item: p.item.clone(), + assessment: file.assessment.id.clone(), + title: file.assessment.title.clone(), + kind: file.assessment.kind, + date: file.assessment.date, + number: p.number, + fingerprint: p.fingerprint.clone(), + }); + } + } + usages.sort_by(|a, b| { + a.date + .cmp(&b.date) + .then(a.assessment.cmp(&b.assessment)) + .then(a.number.cmp(&b.number)) + }); + Ok(History { usages }) + } + + /// Every appearance of one item, oldest first. + /// + /// # Arguments + /// + /// * `uid` - the item's global id. + /// + /// # Returns + /// + /// Matching usages. + pub fn for_item(&self, uid: &str) -> Vec<&Usage> { + self.usages.iter().filter(|u| u.item == uid).collect() + } + + /// The most recent date an item was used. + /// + /// # Arguments + /// + /// * `uid` - the item's global id. + /// + /// # Returns + /// + /// The date, or `None` when never used or never dated. + pub fn last_used(&self, uid: &str) -> Option { + self.for_item(uid).iter().filter_map(|u| u.date).max() + } + + /// How many times an item has been used. + /// + /// # Arguments + /// + /// * `uid` - the item's global id. + /// + /// # Returns + /// + /// The count. + pub fn use_count(&self, uid: &str) -> usize { + self.for_item(uid).len() + } + + /// Whether an item is still inside its reuse cooldown. + /// + /// # Arguments + /// + /// * `uid` - the item's global id. + /// * `cooldown_days` - the minimum gap between uses. + /// * `as_of` - the date of the assessment being assembled. + /// + /// # Returns + /// + /// `true` when the item was used too recently to reuse. + pub fn in_cooldown(&self, uid: &str, cooldown_days: i64, as_of: Date) -> bool { + match self.last_used(uid) { + Some(last) => last.days_until(as_of) < cooldown_days, + None => false, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn history_tracks_last_use_and_cooldown() { + let h = History { + usages: vec![ + Usage { + item: "b::q-1".into(), + assessment: "exam-1".into(), + title: "Exam 1".into(), + kind: Kind::Exam, + date: Some("2025-09-15".parse().unwrap()), + number: 4, + fingerprint: None, + }, + Usage { + item: "b::q-1".into(), + assessment: "exam-3".into(), + title: "Exam 3".into(), + kind: Kind::Exam, + date: Some("2026-02-10".parse().unwrap()), + number: 7, + fingerprint: None, + }, + ], + }; + assert_eq!(h.use_count("b::q-1"), 2); + assert_eq!(h.use_count("b::q-2"), 0); + assert_eq!(h.last_used("b::q-1").unwrap().to_string(), "2026-02-10"); + + let exam_date: Date = "2026-04-23".parse().unwrap(); + // 72 days elapsed, so a 90-day cooldown still blocks it and a 60-day one + // does not. + assert!(h.in_cooldown("b::q-1", 90, exam_date)); + assert!(!h.in_cooldown("b::q-1", 60, exam_date)); + assert!(!h.in_cooldown("b::never-used", 3650, exam_date)); + } +} diff --git a/src/model/layout.rs b/src/model/layout.rs new file mode 100644 index 0000000..1691687 --- /dev/null +++ b/src/model/layout.rs @@ -0,0 +1,99 @@ +//! The on-disk layout of a course directory. +//! +//! Keeping the layout in one type means every command agrees on where things +//! live, and a future rearrangement touches this file only. It is deliberately +//! separate from the course *schema* in [`crate::course`]: this module is about +//! where files sit on disk, not what they contain. + +use std::path::PathBuf; + +use crate::course::COURSE_FILE; +use crate::error::{Error, Result}; + +/// The standard directory layout of a course, resolved from a root. +#[derive(Debug, Clone)] +pub struct Layout { + /// The course root. + pub root: PathBuf, +} + +impl Layout { + /// Builds a layout from a course root directory. + /// + /// # Arguments + /// + /// * `root` - the course directory. + /// + /// # Returns + /// + /// The layout. + pub fn new(root: impl Into) -> Layout { + Layout { root: root.into() } + } + + /// Path to `course.yaml`. + pub fn course_file(&self) -> PathBuf { + self.root.join(COURSE_FILE) + } + + /// Directory holding item bank YAML files. + pub fn banks(&self) -> PathBuf { + self.root.join("banks") + } + + /// Directory holding assessment records. + pub fn assessments(&self) -> PathBuf { + self.root.join("assessments") + } + + /// Directory holding response tables and derived statistics. + pub fn data(&self) -> PathBuf { + self.root.join("data") + } + + /// Directory holding generated reports. + pub fn reports(&self) -> PathBuf { + self.root.join("reports") + } + + /// Directory holding generated exports such as QTI packages. + pub fn build(&self) -> PathBuf { + self.root.join("build") + } + + /// Directory holding emitted JSON Schema files for editor validation. + pub fn schema(&self) -> PathBuf { + self.root.join("schema") + } + + /// Directory holding Typst export templates and their configuration. + /// + /// Unlike the other directories, this one is *not* created by + /// [`Layout::create_all`]. Its absence is meaningful: a course with no + /// `templates/` directory uses the templates compiled into the binary, and + /// creating an empty one on `init` would suggest a customization step is + /// required when it is not. `coursebank template dump` creates it on demand. + pub fn templates(&self) -> PathBuf { + self.root.join("templates") + } + + /// Creates every directory in the layout. + /// + /// # Errors + /// + /// Returns [`Error::Io`] if a directory cannot be created. + pub fn create_all(&self) -> Result<()> { + for dir in [ + self.root.clone(), + self.banks(), + self.assessments(), + self.data(), + self.reports(), + self.build(), + self.schema(), + ] { + std::fs::create_dir_all(&dir).map_err(|e| Error::io(&dir, e))?; + } + Ok(()) + } +} diff --git a/src/util/hash.rs b/src/util/hash.rs index ff2d806..1a61adf 100644 --- a/src/util/hash.rs +++ b/src/util/hash.rs @@ -137,9 +137,9 @@ pub fn sha256(msg: &[u8]) -> [u8; 32] { let mut w = [0u32; 64]; for chunk in data.chunks(64) { - for i in 0..16 { + for (i, w_i) in w.iter_mut().enumerate().take(16) { let j = i * 4; - w[i] = u32::from_be_bytes([chunk[j], chunk[j + 1], chunk[j + 2], chunk[j + 3]]); + *w_i = u32::from_be_bytes([chunk[j], chunk[j + 1], chunk[j + 2], chunk[j + 3]]); } for i in 16..64 { let s0 = w[i - 15].rotate_right(7) ^ w[i - 15].rotate_right(18) ^ (w[i - 15] >> 3); diff --git a/src/util/markup.rs b/src/util/markup.rs index 12e57f7..baa4ca3 100644 --- a/src/util/markup.rs +++ b/src/util/markup.rs @@ -292,7 +292,7 @@ fn wrap_delimited(s: &str, delim: &str, pre: &str, post: &str) -> String { out.push_str(rest); return out; } - Some(j) if j == 0 => { + Some(0) => { // Empty span such as `**`; emit literally and move on. out.push_str(&rest[..i + delim.len()]); rest = after;