From eabc98ad31e821c29b15ca3a919e7bd56dd02e9b Mon Sep 17 00:00:00 2001 From: Alex Maldonado Date: Sat, 26 Sep 2026 01:15:12 -0400 Subject: [PATCH] feat: splitting --- src/analysis/diagnostic.rs | 20 +- src/authoring/jsonschema.rs | 151 ++- src/authoring/lint.rs | 2 +- src/authoring/select.rs | 3 +- src/cli.rs | 120 ++ src/commands.rs | 7 +- src/commands/project.rs | 325 +++++- src/data/responses.rs | 5 +- src/data/store.rs | 68 +- src/export.rs | 2 + src/export/lecture.rs | 3 +- src/export/practice.rs | 23 +- src/export/qti.rs | 2 + src/export/references.rs | 498 +++++++++ src/export/site.rs | 23 +- src/export/typst.rs | 4 +- src/export/typst/diagnostic.rs | 1 + src/lib.rs | 16 +- src/migrate.rs | 1896 ++++++++++++++++++++++++++++++++ src/model.rs | 4 +- src/model/assessment.rs | 21 +- src/model/bank.rs | 75 +- src/model/catalog.rs | 175 ++- src/model/course.rs | 478 +++++++- src/model/course/fragment.rs | 866 +++++++++++++++ src/model/item.rs | 169 ++- src/model/layout.rs | 26 + src/model/seal.rs | 45 +- src/util/yaml.rs | 49 +- 29 files changed, 4843 insertions(+), 234 deletions(-) create mode 100644 src/export/references.rs create mode 100644 src/migrate.rs create mode 100644 src/model/course/fragment.rs diff --git a/src/analysis/diagnostic.rs b/src/analysis/diagnostic.rs index 7a63804..f8cfc10 100644 --- a/src/analysis/diagnostic.rs +++ b/src/analysis/diagnostic.rs @@ -71,7 +71,7 @@ use serde::Serialize; use crate::assessment::AssessmentFile; use crate::catalog::Catalog; use crate::classical::Analysis; -use crate::course::{CourseFile, ReadingRole, Reference}; +use crate::course::{CourseFile, ReadingRole}; use crate::irt::Fit; use crate::item::Citation; use crate::responses::{Response, ResponseSet}; @@ -797,9 +797,9 @@ fn item_readings(course: &CourseFile, citations: &[Citation]) -> Vec ( - reference.label.as_deref().unwrap_or(key).to_string(), + reference.label_or(key).to_string(), Some(reference.title.clone()), - resolve_citation_url(citation, reference), + citation.href(reference), ), None => (citation.display(), None, citation.url.clone()), }; @@ -821,20 +821,6 @@ fn item_readings(course: &CourseFile, citations: &[Citation]) -> Vec Option { - if let Some(url) = &citation.url { - return Some(url.clone()); - } - let path = citation.path.as_deref()?; - let base = reference.base_url.as_deref()?; - Some(match (base.ends_with('/'), path.starts_with('/')) { - (true, true) => format!("{base}{}", &path[1..]), - (false, false) => format!("{base}/{path}"), - _ => format!("{base}{path}"), - }) -} - /// Ranks the lectures behind a student's missed questions. /// /// A lecture earns its place by how many distinct objectives went wrong in it, diff --git a/src/authoring/jsonschema.rs b/src/authoring/jsonschema.rs index ce3dc32..0796b59 100644 --- a/src/authoring/jsonschema.rs +++ b/src/authoring/jsonschema.rs @@ -31,6 +31,12 @@ const BASE: &str = "https://coursebank.dev/schema"; pub enum Kind { /// `course.yaml`. Course, + /// `references.yaml`. + References, + /// `lectures/*.yaml`. + Lecture, + /// `objectives/*.yaml`. + Objective, /// `banks/*.yaml`. Bank, /// `assessments/*.yaml`. @@ -38,13 +44,23 @@ pub enum Kind { } impl Kind { - /// All three kinds. - pub const ALL: [Kind; 3] = [Kind::Course, Kind::Bank, Kind::Assessment]; + /// Every kind. + pub const ALL: [Kind; 6] = [ + Kind::Course, + Kind::References, + Kind::Lecture, + Kind::Objective, + Kind::Bank, + Kind::Assessment, + ]; /// The file name a schema is written to. pub fn filename(self) -> &'static str { match self { Kind::Course => "course.schema.json", + Kind::References => "references.schema.json", + Kind::Lecture => "lecture.schema.json", + Kind::Objective => "objective.schema.json", Kind::Bank => "bank.schema.json", Kind::Assessment => "assessment.schema.json", } @@ -80,12 +96,15 @@ impl Kind { pub fn schema(kind: Kind) -> Value { match kind { Kind::Course => course_schema(), + Kind::References => references_schema(), + Kind::Lecture => lecture_fragment_schema(), + Kind::Objective => objective_fragment_schema(), Kind::Bank => bank_schema(), Kind::Assessment => assessment_schema(), } } -/// Writes all three schemas to a directory. +/// Writes every schema to a directory. /// /// # Arguments /// @@ -328,6 +347,13 @@ fn lecture_schema() -> Value { "date": date("Date delivered."), "unit": { "type": "string", "description": "Unit id." }, "slides_url": { "type": "string" }, + "teaches": { + "type": "array", + "description": "The objectives this session develops. Each named objective gains \ + this lecture in its `lectures` list when the course is loaded, \ + so the pair is declared once, here, while planning the lecture.", + "items": { "type": "string" } + }, "readings": { "type": "array", "description": "Readings assigned with this lecture, in the order you assign \ @@ -434,7 +460,23 @@ fn reference_schema() -> Value { "volume": { "type": "string" }, "issue": { "type": "string" }, "pages": { "type": "string", "description": "Pages of the work, not of a reading." }, - "doi": { "type": "string", "description": "Bare DOI: 10.1038/nature12373." }, + "doi": { + "type": "string", + "pattern": "^(doi:|https?://(dx\\.)?doi\\.org/)?10\\.", + "description": "Bare DOI: 10.1038/nature12373. For a manuscript this is \ + usually the only link worth storing, since a reading list \ + resolves it to doi.org." + }, + "arxiv": { "type": "string", "description": "Bare arXiv id: 2301.00001." }, + "pmcid": { + "type": "string", + "description": "PubMed Central id, which hosts the full text: PMC3084216." + }, + "pmid": { + "type": "string", + "pattern": "^[0-9]+$", + "description": "PubMed id, which hosts a record about the work: 21471563." + }, "isbn": { "type": "string" }, "url": { "type": "string", "description": "Canonical URL for the whole work." }, "base_url": { @@ -568,6 +610,94 @@ fn course_schema() -> Value { }) } +/// The schema for `references.yaml`. +fn references_schema() -> Value { + json!({ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": format!("{BASE}/references.schema.json"), + "title": "coursebank references file", + "description": "The works the course cites, by citation key. One fragment of the \ + course file; see course.schema.json for the whole.", + "type": "object", + "additionalProperties": false, + "properties": { + "schema_version": { + "type": ["string", "number"], + "description": format!("Format version; currently {SCHEMA_VERSION}. Declared in \ + course.yaml; fragments inherit it.") + }, + "references": { + "type": "object", + "description": "Works by citation key.", + "additionalProperties": reference_schema() + } + } + }) +} + +/// The schema for one file under `lectures/`. +fn lecture_fragment_schema() -> Value { + json!({ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": format!("{BASE}/lecture.schema.json"), + "title": "coursebank lecture file", + "description": "One session: its readings, and the objectives it develops. A fragment \ + of the course file, merged on load.", + "type": "object", + "required": ["lectures"], + "additionalProperties": false, + "properties": { + "schema_version": { + "type": ["string", "number"], + "description": "Declared in course.yaml; fragments inherit it." + }, + "lectures": { + "type": "object", + "description": "Keyed by lecture id, conventionally one entry per file.", + "additionalProperties": lecture_schema() + } + } + }) +} + +/// The schema for one file under `objectives/`. +fn objective_fragment_schema() -> Value { + json!({ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": format!("{BASE}/objective.schema.json"), + "title": "coursebank objective file", + "description": "One learning objective and the learning targets it decomposes into. A \ + fragment of the course file, merged on load.", + "type": "object", + "required": ["learning_objectives"], + "additionalProperties": false, + "properties": { + "schema_version": { + "type": ["string", "number"], + "description": "Declared in course.yaml; fragments inherit it." + }, + "learning_objectives": { + "type": "object", + "description": "Keyed by objective id, conventionally one entry per file. Its \ + `lectures` list is derived from each lecture's `teaches`, so \ + leave it out unless you prefer to declare it here.", + "additionalProperties": objective_schema() + }, + "learning_targets": { + "type": "object", + "description": "The targets of this file's objective, by id. A target with no \ + `lectures` of its own inherits its objective's.", + "additionalProperties": target_schema() + }, + "stimuli": { + "type": "object", + "description": "Shared passages, figures, or data that several items refer to.", + "additionalProperties": stimulus_schema() + } + } + }) +} + /// One option's schema. /// /// Split out from [`item_schema`] rather than inlined, because `serde_json`'s @@ -583,9 +713,11 @@ fn option_schema() -> Value { "properties": { "id": { "type": "string", - "pattern": "^[A-H]$", - "description": "Option letter. Identity, not print position — shuffled forms \ - relabel on the way out." + "pattern": "^(o-[a-z0-9]+(-[a-z0-9]+)*|[A-H])$", + "description": "Option id, unique within the item: `o-fourth-line`. An \ + identity, not a print position — shuffled forms relabel on the \ + way out. A single letter A-H is the pre-2.0 form; \ + `coursebank migrate options` renames it." }, "text": text("The option as a student reads it."), "correct": { "type": "boolean" }, @@ -1260,7 +1392,8 @@ mod tests { assert_eq!(props["options"]["maxItems"], 8); assert_eq!( props["options"]["items"]["properties"]["id"]["pattern"], - "^[A-H]$" + // Either form: the 2.0 name, or the letter it replaces. + "^(o-[a-z0-9]+(-[a-z0-9]+)*|[A-H])$" ); } @@ -1283,7 +1416,7 @@ mod tests { let dir = std::env::temp_dir().join(format!("cb-schema-{}", std::process::id())); std::fs::remove_dir_all(&dir).ok(); let written = write_all(&dir).unwrap(); - assert_eq!(written.len(), 3); + assert_eq!(written.len(), Kind::ALL.len()); for path in &written { assert!(path.exists()); let text = std::fs::read_to_string(path).unwrap(); diff --git a/src/authoring/lint.rs b/src/authoring/lint.rs index 6b78b45..6760b5f 100644 --- a/src/authoring/lint.rs +++ b/src/authoring/lint.rs @@ -1196,7 +1196,7 @@ mod tests { fn entry(yaml: &str) -> Entry { let item: Item = serde_yaml_ng::from_str(yaml).expect("item parses"); Entry { - uid: format!("b::{}", item.id), + uid: item.id.clone(), bank: "b".into(), path: PathBuf::from("b.yaml"), index: 0, diff --git a/src/authoring/select.rs b/src/authoring/select.rs index a81fb9e..286b607 100644 --- a/src/authoring/select.rs +++ b/src/authoring/select.rs @@ -469,7 +469,8 @@ pub fn to_record( items.push(Placement { number, item: uid.clone(), - version: Some(e.item.version), + version: None, + stem_digest: Some(e.item.stem_digest()), fingerprint: Some(e.item.fingerprint()), points: Some(e.item.points(default_points)), bonus: is_bonus || e.item.bonus, diff --git a/src/cli.rs b/src/cli.rs index ab56996..5f11f19 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -24,6 +24,7 @@ use coursebank::assessment::{Kind as AssessmentKind, Platform}; use coursebank::catalog::Severity; use coursebank::item::IrtModel; use coursebank::lecture::Style as PageStyle; +use coursebank::references; use coursebank::store; /// Manage course item banks, assessments, and the analysis that comes back. @@ -48,6 +49,15 @@ pub(crate) struct Cli { pub(crate) enum Command { /// Create a new course directory. Init(InitArgs), + /// Inspect the course file. + #[command(subcommand)] + Course(CourseCommand), + /// One-time conversions from an older layout. + #[command(subcommand)] + Migrate(MigrateCommand), + /// Work with the bibliography. + #[command(subcommand)] + References(ReferencesCommand), /// Write JSON Schemas so your editor can validate the YAML as you type. Schema, /// Check every file for problems that must be fixed. @@ -93,6 +103,116 @@ pub(crate) enum Command { Data, } +/// `course`: the course file itself, which may be one file or many. +#[derive(Debug, Subcommand)] +pub(crate) enum CourseCommand { + /// List the files the course is assembled from. + Files, + /// Print the merged course, or write it to a file. + /// + /// Nothing reads what this writes. It exists so you can see what the + /// fragments add up to, and diff two revisions of a course that no longer + /// lives in one file. + Build { + /// Output path; prints to stdout when omitted. + #[arg(long)] + out: Option, + }, + /// Say which file defines an id. + Where { + /// A unit, lecture, objective, target, reference, or stimulus id. + id: String, + }, +} + +/// `migrate`: the one-time conversions, grouped so they are findable together. +#[derive(Debug, Subcommand)] +pub(crate) enum MigrateCommand { + /// Split one course.yaml into references.yaml, lectures/, and objectives/. + /// + /// The original is kept as course.yaml.bak, and the result is reassembled + /// and compared against it before the command reports success. + Split { + /// Show what would be written, and write nothing. + #[arg(long)] + dry_run: bool, + }, + /// Drop `version:` and `history:`, which 2.0 ignores. + /// + /// A stem's text is its identity: reword it and it is a new item with a new + /// id and `supersedes:` pointing back. `validate` enforces that against + /// every seal, so what a version number used to hint at is now checked. + Stems { + /// Show what would change, and write nothing. + #[arg(long)] + dry_run: bool, + }, + /// Rewrite option letters as names derived from the option text. + /// + /// A letter is a position, and a position in a field that pooled + /// statistics and `credit_overrides` join on is a bug waiting for someone + /// to reorder a YAML block. Run with --dry-run first: the names land in + /// the response store, so they are as permanent as an item id. + Options { + /// Show the derived names, and write nothing. + #[arg(long)] + dry_run: bool, + }, + /// Rewrite pre-2.0 `bank::item` ids as the item ids they name. + /// + /// Touches assessment records, seals, and the response store. Everything + /// keeps working unmigrated — an old id still resolves — but a store + /// holding both forms groups one question into two for anything reading the + /// Parquet without this tool. + Ids { + /// Show what would change, and write nothing. + #[arg(long)] + dry_run: bool, + }, +} + +/// `references`: the bibliography, and the formats other tools read it in. +#[derive(Debug, Subcommand)] +pub(crate) enum ReferencesCommand { + /// List every work, with the link a reading list would use. + /// + /// The column that matters is the last one: a work with no link is one a + /// student cannot reach from a report, which for a manuscript usually means + /// its DOI is missing. + List, + /// Write the bibliography in a citation format. + Export { + /// Which format to write. + #[arg(long, value_enum, default_value = "hayagriva")] + format: ReferenceFormat, + /// Output path; prints to stdout when omitted. + #[arg(long)] + out: Option, + }, +} + +/// The citation formats `references export` can write. +#[derive(Debug, Clone, Copy, ValueEnum)] +pub(crate) enum ReferenceFormat { + /// Hayagriva YAML, which Typst reads natively. + Hayagriva, + /// CSL-JSON, for Zotero, Pandoc, and CSL processors. + CslJson, + /// BibTeX. + Bibtex, +} + +impl ReferenceFormat { + /// The library-side format. + pub(crate) fn as_format(self) -> references::Format { + match self { + ReferenceFormat::Hayagriva => references::Format::Hayagriva, + ReferenceFormat::CslJson => references::Format::CslJson, + ReferenceFormat::Bibtex => references::Format::Bibtex, + } + } +} + #[derive(Debug, Args)] pub(crate) struct InitArgs { /// Course code, e.g. "BIOSC 1540". diff --git a/src/commands.rs b/src/commands.rs index af2bad0..9c34908 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -8,8 +8,8 @@ //! calls the matching handler. The handlers themselves live in submodules that //! follow the workflow described in the crate documentation: //! -//! - [`project`] — set up and check a course: `init`, `schema`, `validate`, -//! `lint`, `catalog`. +//! - [`project`] — set up and check a course: `init`, `course`, `schema`, +//! `validate`, `lint`, `catalog`. //! - [`lectures`] — render a lecture's reading list and check what backs each //! objective: `lecture`. //! - [`banks`] — manage items and build assessments: `bank`, `assessment`, @@ -55,6 +55,9 @@ pub(crate) enum Outcome { pub(crate) fn run(cli: &Cli) -> Result { match &cli.command { Command::Init(args) => project::init(cli, args), + Command::Course(sub) => project::course(cli, sub), + Command::References(sub) => project::references(cli, sub), + Command::Migrate(sub) => project::migrate(cli, sub), Command::Schema => project::schema(cli), Command::Validate => project::validate(cli), Command::Lint(args) => project::lint(cli, args), diff --git a/src/commands/project.rs b/src/commands/project.rs index 1619f90..5e912e8 100644 --- a/src/commands/project.rs +++ b/src/commands/project.rs @@ -5,9 +5,10 @@ //! Setting up a course and checking it stays well-formed. //! //! These are the commands you reach for before and around authoring: create the -//! directory (`init`), write editor schemas (`schema`), and run the two kinds of -//! checking — [`validate`] for problems that must be fixed and [`lint`] for -//! item-writing guidance. [`catalog`] summarizes the pool that results. +//! directory (`init`), see and split the course file (`course`), export the +//! bibliography (`references`), write editor schemas (`schema`), and run the two +//! kinds of checking — [`validate`] for problems that must be fixed and [`lint`] +//! for item-writing guidance. [`catalog`] summarizes the pool that results. use std::collections::{BTreeMap, BTreeSet}; use std::fs; @@ -15,15 +16,20 @@ use std::path::Path; use coursebank::assessment::AssessmentFile; use coursebank::bank::BankFile; +use coursebank::course::fragment::{self, Section}; 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::migrate; +use coursebank::references; use coursebank::taxonomy::{Level, Tier}; use coursebank::yaml; -use crate::cli::{CatalogArgs, Cli, InitArgs, LintArgs}; +use crate::cli::{ + CatalogArgs, Cli, CourseCommand, InitArgs, LintArgs, MigrateCommand, ReferencesCommand, +}; use crate::commands::Outcome; use crate::helpers::{load, truncate}; @@ -77,13 +83,315 @@ pub(crate) fn init(cli: &Cli, args: &InitArgs) -> Result { write_gitignore(&cli.course.join(".gitignore"))?; println!( - "\nNext: edit {} to add your learning objectives, their targets, and your\n lectures, then\n \ - coursebank bank new unit-1 --title \"Unit 1\"\n coursebank validate", - COURSE_FILE + "\nNext: edit {COURSE_FILE} to add your learning objectives, their targets, and\n \ + your lectures, then\n coursebank bank new unit-1 --title \"Unit 1\"\n \ + coursebank validate\n\nOnce {COURSE_FILE} is more than you want to scroll, \ + `coursebank migrate split`\n moves each lecture and objective into its own file under \ + lectures/ and\n objectives/, and every command goes on reading the course as one." ); Ok(Outcome::Ok) } +/// `course`: inspect the course file, or split it into fragments. +pub(crate) fn course(cli: &Cli, sub: &CourseCommand) -> Result { + match sub { + CourseCommand::Files => course_files(cli), + CourseCommand::Build { out } => course_build(cli, out.as_deref()), + CourseCommand::Where { id } => course_where(cli, id), + } +} + +/// `migrate`: the one-time layout conversions. +pub(crate) fn migrate(cli: &Cli, sub: &MigrateCommand) -> Result { + match sub { + MigrateCommand::Split { dry_run } => course_split(cli, *dry_run), + MigrateCommand::Ids { dry_run } => migrate_ids(cli, *dry_run), + MigrateCommand::Options { dry_run } => migrate_options(cli, *dry_run), + MigrateCommand::Stems { dry_run } => migrate_stems(cli, *dry_run), + } +} + +/// Drops the version fields 2.0 ignores. +fn migrate_stems(cli: &Cli, dry_run: bool) -> Result { + let touched = migrate::stems(&cli.course, !dry_run)?; + if touched.is_empty() { + println!("nothing to migrate: no `version:` or `history:` left to drop"); + return Ok(Outcome::Ok); + } + for (path, n) in &touched { + println!(" {:<44} {n:>5} line(s) dropped", path.display()); + } + if dry_run { + println!("\nnothing written"); + return Ok(Outcome::Ok); + } + println!( + "\nrewrote {} file(s)\n\nNext:\n coursebank validate\n\nFrom here, rewording a stem \ + is an error rather than a version bump: give the new\n wording a new id and \ + `supersedes:` the old one.", + touched.len() + ); + Ok(Outcome::Ok) +} + +/// Rewrites option letters as names, showing every name before writing. +fn migrate_options(cli: &Cli, dry_run: bool) -> Result { + let (map, problems) = migrate::options_plan(&cli.course)?; + + for (item, options) in &map { + println!("{item}"); + for (letter, name) in options { + println!(" {letter} -> {name}"); + } + } + + if !problems.is_empty() { + println!("\n{} item(s) need naming by hand:", problems.len()); + for problem in &problems { + println!(" - {problem}"); + } + } + if map.is_empty() { + println!("nothing to migrate: every option is already named"); + return Ok(Outcome::Ok); + } + + let touched = migrate::apply_options(&cli.course, &map, !dry_run)?; + println!(); + for (path, n) in &touched { + println!(" {:<44} {n:>5} rename(s)", path.display()); + } + if dry_run { + println!("\nnothing written"); + return Ok(if problems.is_empty() { + Outcome::Ok + } else { + Outcome::Findings + }); + } + println!( + "\nrewrote {} file(s)\n\nSeals keep their letters on purpose; see `coursebank migrate \ + --help`.\nNext:\n coursebank validate\n coursebank lint", + touched.len() + ); + Ok(if problems.is_empty() { + Outcome::Ok + } else { + Outcome::Findings + }) +} + +/// Rewrites pre-2.0 bank-qualified item ids everywhere they are stored. +fn migrate_ids(cli: &Cli, dry_run: bool) -> Result { + let files = migrate::qualified_ids(&cli.course)?; + for (path, n, _) in &files { + println!(" {:<44} {n:>5} id(s)", path.display()); + } + if !dry_run { + migrate::apply_ids(&cli.course, &files)?; + } + + let data = migrate::store_ids(&cli.course, !dry_run)?; + for (path, n) in &data { + println!(" {:<44} {n:>5} row(s)", path.display()); + } + + if files.is_empty() && data.is_empty() { + println!("nothing to migrate: every item id already names the item course-wide"); + return Ok(Outcome::Ok); + } + if dry_run { + println!("\nnothing written"); + return Ok(Outcome::Ok); + } + println!( + "\nrewrote {} file(s) and {} data file(s)\n\nNext:\n coursebank validate\n \ + coursebank analyze items --all", + files.len(), + data.len() + ); + Ok(Outcome::Ok) +} + +/// Lists the fragments a course is assembled from, with what each defines. +fn course_files(cli: &Cli) -> Result { + let layout = Layout::new(&cli.course); + let course = CourseFile::load_dir(&cli.course)?; + + for (path, role) in fragment::files(&layout)? { + if !path.exists() { + continue; + } + let shown = path.strip_prefix(&cli.course).unwrap_or(&path); + let mut defines: Vec = Vec::new(); + for section in Section::ALL { + let n = course + .origins + .iter() + .filter(|((s, _), p)| *s == section && p.as_path() == shown) + .count(); + if n == 0 { + continue; + } + defines.push(match section { + // These are declared once for the whole course, so a count + // would always be 1 and would read as though it could be more. + Section::Course | Section::Policy => section.key().to_string(), + _ => format!("{n} {}", section.key()), + }); + } + println!( + "{:<40} {:<11} {}", + shown.display(), + role.label(), + defines.join(", ") + ); + } + Ok(Outcome::Ok) +} + +/// Prints or writes the merged course. +fn course_build(cli: &Cli, out: Option<&Path>) -> Result { + let course = CourseFile::load_dir(&cli.course)?; + match out { + Some(path) => { + course.write_resolved(path)?; + println!( + "wrote {} from {} file(s)", + path.display(), + course.fragment_paths().len() + ); + } + None => print!("{}", yaml::to_string(&course)?), + } + Ok(Outcome::Ok) +} + +/// Says which file defines an id. +fn course_where(cli: &Cli, id: &str) -> Result { + let course = CourseFile::load_dir(&cli.course)?; + match course.origin(id) { + Some((section, path)) => { + println!( + "{} defines `{id}` under `{}`", + path.display(), + section.key() + ); + Ok(Outcome::Ok) + } + None => Err(Error::Unresolved { + kind: "id", + id: id.to_string(), + context: Some(cli.course.display().to_string()), + }), + } +} + +/// Splits `course.yaml` into fragments, then checks the result reassembles. +fn course_split(cli: &Cli, dry_run: bool) -> Result { + let plan = migrate::split_plan(&cli.course)?; + + println!("{} file(s):", plan.files.len()); + for (path, lines) in plan.lines() { + let summary = plan + .files + .iter() + .find(|f| f.path == path) + .map(|f| f.summary.clone()) + .unwrap_or_default(); + println!(" {:<44} {lines:>5} lines {summary}", path.display()); + } + for note in &plan.notes { + println!("\nnote: {note}"); + } + + if dry_run { + println!("\nnothing written"); + return Ok(Outcome::Ok); + } + + // Loaded before anything is written, since it is the thing the result is + // checked against. + let before = CourseFile::load(&Layout::new(&cli.course).course_file())?; + let written = migrate::apply(&cli.course, &plan)?; + println!("\nwrote {} file(s)", written.len()); + + let after = CourseFile::load_dir(&cli.course)?; + let diffs = migrate::differences(&before, &after)?; + if diffs.is_empty() { + println!( + "reassembled and compared against {COURSE_FILE}.bak: identical\n\nNext:\n \ + coursebank validate\n coursebank schema\n git add -A && git diff --cached --stat" + ); + return Ok(Outcome::Ok); + } + + println!( + "\n{} difference(s) between the original and the reassembled course:", + diffs.len() + ); + for diff in &diffs { + println!(" - {diff}"); + } + println!( + "\nThe original is at {COURSE_FILE}.bak. Restore it with\n mv {COURSE_FILE}.bak \ + {COURSE_FILE} && rm -r lectures objectives {}", + fragment::REFERENCES_FILE + ); + Ok(Outcome::Findings) +} + +/// `references`: list the bibliography, or export it in a citation format. +pub(crate) fn references(cli: &Cli, sub: &ReferencesCommand) -> Result { + let course = CourseFile::load_dir(&cli.course)?; + + match sub { + ReferencesCommand::List => { + let mut unreachable = 0; + for (key, reference) in &course.references { + let link = match reference.href(None, None) { + Some(url) => url, + None => { + unreachable += 1; + "(no link)".to_string() + } + }; + println!( + "{:<28} {:<10} {:<9} {:<6} {link}", + truncate(key, 27), + format!("{:?}", reference.kind).to_lowercase(), + format!("{:?}", reference.role).to_lowercase(), + reference.label_or("-"), + ); + } + if unreachable > 0 && !cli.quiet { + println!( + "\n{unreachable} work(s) with no link. A student report can name one but \ + cannot send anyone to it; for a manuscript, add its `doi`." + ); + } + Ok(Outcome::Ok) + } + ReferencesCommand::Export { format, out } => { + let format = format.as_format(); + let text = references::render(&course, format)?; + match out { + Some(path) => { + yaml::write_text(path, &text)?; + println!( + "wrote {} ({} work(s) as {})", + path.display(), + course.references.len(), + format.label() + ); + } + None => print!("{text}"), + } + Ok(Outcome::Ok) + } + } +} + /// What reconciling [`GITIGNORE`] against a file already on disk would do. struct GitignoreMerge { /// The file to write. Identical to the input when nothing was missing. @@ -260,6 +568,9 @@ pub(crate) fn validate(cli: &Cli) -> Result { } } + let seals = coursebank::seal::SealFile::load_all(&catalog.layout.seals())?; + all.extend(catalog.validate_seals(&seals)); + if all.is_empty() { if !cli.quiet { println!( diff --git a/src/data/responses.rs b/src/data/responses.rs index 53f0684..e639460 100644 --- a/src/data/responses.rs +++ b/src/data/responses.rs @@ -767,7 +767,10 @@ impl FlatResponse { email: none_if_empty(&self.email), section: none_if_empty(&self.section), item_number: self.item_number, - item_ref: none_if_empty(&self.item_ref), + // Canonicalized on read, so a term ingested before 2.0 pools with + // one ingested after it instead of splitting into two items. + item_ref: none_if_empty(&self.item_ref) + .map(|id| crate::item::canonical_id(&id).to_string()), item_version: if self.item_version == 0 { None } else { diff --git a/src/data/store.rs b/src/data/store.rs index ffb5cd9..063d513 100644 --- a/src/data/store.rs +++ b/src/data/store.rs @@ -316,6 +316,70 @@ pub fn read_path(path: &Path) -> Result { } } +/// Reads a response file without turning its rows into [`Response`]s. +/// +/// What a migration wants: the rows exactly as they sit on disk, so rewriting +/// one column cannot disturb another through a round trip. +/// +/// # Arguments +/// +/// * `path` - the file to read. +/// +/// # Returns +/// +/// The rows. +/// +/// # Errors +/// +/// Returns [`Error::Other`] for an unrecognized extension, [`Error::Csv`] or +/// [`Error::Other`] on a parse failure, and [`Error::FeatureDisabled`] for +/// Parquet without the feature. +pub fn read_flat(path: &Path) -> Result> { + match Format::from_path(path) { + Some(Format::Csv) => { + let mut r = csv::Reader::from_path(path).map_err(|e| Error::Csv { + path: path.to_path_buf(), + source: e, + })?; + let mut out = Vec::new(); + for rec in r.deserialize::() { + out.push(rec.map_err(|e| Error::Csv { + path: path.to_path_buf(), + source: e, + })?); + } + Ok(out) + } + Some(Format::Parquet) => crate::store_parquet::read(path), + None => Err(Error::Other(format!( + "{} is not a response file; expected a .parquet or .csv", + path.display() + ))), + } +} + +/// Writes flat responses back to the file they came from. +/// +/// # Arguments +/// +/// * `path` - the destination, whose extension picks the format. +/// * `rows` - the rows. +/// +/// # Errors +/// +/// Returns [`Error::Other`] for an unrecognized extension and +/// [`Error::FeatureDisabled`] for Parquet without the feature. +pub fn write_flat(path: &Path, rows: &[FlatResponse]) -> Result<()> { + match Format::from_path(path) { + Some(Format::Csv) => write_csv(path, rows), + Some(Format::Parquet) => write_parquet(path, rows), + None => Err(Error::Other(format!( + "{} is not a response file; expected a .parquet or .csv", + path.display() + ))), + } +} + /// Writes flat responses as CSV. /// /// # Arguments @@ -591,7 +655,9 @@ mod tests { let back = store.read("BIOSC1540/2026s/exam-4").unwrap(); assert_eq!(back.rows.len(), 2); - assert_eq!(back.rows[0].item_ref.as_deref(), Some("bank::q-x-001")); + // Canonicalized on the way in: the row was written with a pre-2.0 + // `bank::item` key, and reading it yields the item it names. + assert_eq!(back.rows[0].item_ref.as_deref(), Some("q-x-001")); assert_eq!(back.rows[0].selected, vec!["C".to_string()]); assert_eq!(back.rows[0].learning_targets, vec!["lo-a".to_string()]); diff --git a/src/export.rs b/src/export.rs index c8bd215..c00dd60 100644 --- a/src/export.rs +++ b/src/export.rs @@ -12,6 +12,7 @@ //! | [`site`] | a Quarto partial and an encrypted bundle | a course page with password-gated solutions | //! | [`report`] | Markdown and HTML | students, and yourself | //! | [`lecture`] | Markdown | the reading list on the course website | +//! | [`references`] | Hayagriva, CSL-JSON, BibTeX | Typst, Zotero, LaTeX | //! //! [`qti`] and [`typst`] share one rule that is easy to get wrong: a form's answer //! key must be generated from the same permutation that produced its question @@ -26,6 +27,7 @@ pub mod lecture; pub mod practice; pub mod qti; +pub mod references; pub mod report; pub mod site; pub mod typst; diff --git a/src/export/lecture.rs b/src/export/lecture.rs index cba635d..b5b84a9 100644 --- a/src/export/lecture.rs +++ b/src/export/lecture.rs @@ -374,7 +374,7 @@ fn entry( /// /// A linked citation when the location has a URL, and a plain one when it does not. fn heading(reading: &Reading, key: &str, reference: &Reference, style: Style) -> String { - let label = reference.label.as_deref().unwrap_or(key); + let label = reference.label_or(key); let locator = reading.locator.as_deref().unwrap_or(""); let linked = match reading.resolve_url(reference) { Some(url) if !locator.is_empty() => format!("[{locator}]({url})"), @@ -428,6 +428,7 @@ mod tests { date: None, unit: None, slides_url: None, + teaches: Vec::new(), readings: vec![ Reading { reference: Some("kuriyan2013molecules".into()), diff --git a/src/export/practice.rs b/src/export/practice.rs index 5ce8852..841198b 100644 --- a/src/export/practice.rs +++ b/src/export/practice.rs @@ -30,7 +30,7 @@ use crate::assessment::{AssessmentFile, Form, Placement}; use crate::catalog::Catalog; -use crate::course::{CourseFile, Reference}; +use crate::course::CourseFile; use crate::error::Result; use crate::item::{Choice, Citation, Item}; use crate::markup; @@ -427,9 +427,9 @@ fn cite(course: &CourseFile, citation: &Citation) -> String { let Some(reference) = course.references.get(key) else { return citation.display(); }; - let label = reference.label.as_deref().unwrap_or(key); + let label = reference.label_or(key); let locator = citation.locator.as_deref().unwrap_or(""); - match resolve_url(citation, reference) { + match citation.href(reference) { Some(url) if !locator.is_empty() => format!("`{label}` [{locator}]({url})"), Some(url) => format!("`{label}` [{}]({url})", reference.title), None if !locator.is_empty() => format!("`{label}` {locator}"), @@ -437,21 +437,6 @@ fn cite(course: &CourseFile, citation: &Citation) -> String { } } -/// The URL for a citation: its own `url`, else the reference `base_url` joined with -/// the citation `path`. -fn resolve_url(citation: &Citation, reference: &Reference) -> Option { - if let Some(url) = &citation.url { - return Some(url.clone()); - } - let path = citation.path.as_deref()?; - let base = reference.base_url.as_deref()?; - Some(match (base.ends_with('/'), path.starts_with('/')) { - (true, true) => format!("{base}{}", &path[1..]), - (false, false) => format!("{base}/{path}"), - _ => format!("{base}{path}"), - }) -} - /// The `## Question N` heading, marking a bonus item. fn heading(number: usize, placement: &Placement) -> String { let bonus = if placement.bonus { " (bonus)" } else { "" }; @@ -593,6 +578,7 @@ items: number: 1, item: "l11::q-enthalpy-001".into(), version: None, + stem_digest: None, fingerprint: None, points: Some(1.0), bonus: false, @@ -608,6 +594,7 @@ items: number: 2, item: "l11::q-enthalpy-op-001".into(), version: None, + stem_digest: None, fingerprint: None, points: Some(2.0), bonus: false, diff --git a/src/export/qti.rs b/src/export/qti.rs index cba14ef..c2646b7 100644 --- a/src/export/qti.rs +++ b/src/export/qti.rs @@ -1389,6 +1389,7 @@ items: number: 1, item: "b::q-mcq".into(), version: None, + stem_digest: None, fingerprint: None, points: Some(1.0), bonus: false, @@ -1404,6 +1405,7 @@ items: number: 2, item: "b::q-open".into(), version: None, + stem_digest: None, fingerprint: None, points: Some(2.0), bonus: false, diff --git a/src/export/references.rs b/src/export/references.rs new file mode 100644 index 0000000..6b1e09b --- /dev/null +++ b/src/export/references.rs @@ -0,0 +1,498 @@ +// SPDX-License-Identifier: Prosperity-3.0.0 +// Copyright Scientific Computing Studio +// Source: https://git.scient.ing/education/coursebank + +//! The bibliography, in the formats other tools read. +//! +//! `references.yaml` is the authoritative copy, and it is shaped for a reading +//! list: it carries a `label` that reports print, and a `role` saying whether +//! the course requires the work or offers it as background. No general citation +//! format has either field, which is why the course keeps its own. +//! +//! What the other formats are for is everything downstream of the reading list: +//! +//! | Format | Read by | Gets you | +//! |:--|:--|:--| +//! | [`Format::Hayagriva`] | Typst | real citations in a printed exam or report | +//! | [`Format::CslJson`] | Zotero, Pandoc, CSL processors | a bibliography in any style | +//! | [`Format::Bibtex`] | LaTeX, most reference managers | the lowest common denominator | +//! +//! All three are generated. The argument is the same one the lecture reading +//! list makes: two copies of a citation drift within a term, and one copy plus a +//! build step does not. +//! +//! # What does not survive the trip +//! +//! `label`, `role`, `base_url`, and `note` have nowhere to go in any of the +//! three, so they stay behind. That is the reason this is an export rather than +//! a migration: the course file is not recoverable from its own bibliography +//! export, and nothing reads these files back in. + +use std::collections::BTreeMap; + +use serde::Serialize; +use serde_json::{Value, json}; + +use crate::course::{CourseFile, Reference, ReferenceKind}; +use crate::error::Result; +use crate::yaml; + +/// Which citation format to write. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Format { + /// Hayagriva YAML, which Typst's `#bibliography` reads natively. + Hayagriva, + /// CSL-JSON, which Zotero, Pandoc, and every CSL processor read. + CslJson, + /// BibTeX, for LaTeX and for reference managers that read nothing else. + Bibtex, +} + +impl Format { + /// The conventional file extension. + pub fn extension(self) -> &'static str { + match self { + Format::Hayagriva => "yml", + Format::CslJson => "json", + Format::Bibtex => "bib", + } + } + + /// The name used on the command line. + pub fn label(self) -> &'static str { + match self { + Format::Hayagriva => "hayagriva", + Format::CslJson => "csl-json", + Format::Bibtex => "bibtex", + } + } +} + +/// Renders a course's bibliography. +/// +/// # Arguments +/// +/// * `course` - the loaded course. +/// * `format` - which format to write. +/// +/// # Returns +/// +/// The document, keyed or ordered by citation key so the output is stable +/// between runs. +/// +/// # Errors +/// +/// Returns [`crate::error::Error::Other`] if the intermediate structure cannot +/// be serialized. +pub fn render(course: &CourseFile, format: Format) -> Result { + match format { + Format::Hayagriva => hayagriva(course), + Format::CslJson => csl_json(course), + Format::Bibtex => Ok(bibtex(course)), + } +} + +// --- Hayagriva --- + +/// One Hayagriva entry. +#[derive(Debug, Serialize)] +struct Entry { + #[serde(rename = "type")] + kind: &'static str, + title: String, + #[serde(skip_serializing_if = "Vec::is_empty")] + author: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + date: Option, + #[serde(skip_serializing_if = "Option::is_none")] + edition: Option, + #[serde(skip_serializing_if = "Option::is_none")] + publisher: Option, + #[serde(rename = "page-range", skip_serializing_if = "Option::is_none")] + page_range: Option, + #[serde(rename = "serial-number", skip_serializing_if = "BTreeMap::is_empty")] + serial_number: BTreeMap<&'static str, String>, + #[serde(skip_serializing_if = "Option::is_none")] + url: Option, + #[serde(skip_serializing_if = "Option::is_none")] + parent: Option, +} + +/// The container a Hayagriva entry sits inside. +#[derive(Debug, Serialize)] +struct Parent { + #[serde(rename = "type")] + kind: &'static str, + title: String, + #[serde(skip_serializing_if = "Option::is_none")] + volume: Option, + #[serde(skip_serializing_if = "Option::is_none")] + issue: Option, + #[serde(skip_serializing_if = "Option::is_none")] + publisher: Option, +} + +/// Renders the bibliography as Hayagriva YAML. +fn hayagriva(course: &CourseFile) -> Result { + let entries: BTreeMap<&String, Entry> = course + .references + .iter() + .map(|(key, reference)| (key, entry(reference))) + .collect(); + yaml::to_string(&entries) +} + +/// Maps one reference onto a Hayagriva entry. +fn entry(reference: &Reference) -> Entry { + let mut serial: BTreeMap<&'static str, String> = BTreeMap::new(); + for (field, value) in [ + ("doi", &reference.doi), + ("isbn", &reference.isbn), + ("arxiv", &reference.arxiv), + ("pmid", &reference.pmid), + ("pmcid", &reference.pmcid), + ] { + if let Some(value) = value { + serial.insert(field, value.clone()); + } + } + + // A chapter sits in a book and an article sits in a periodical, and + // Hayagriva wants that said with a parent rather than a flat field. + let parent = reference + .container + .as_ref() + .map(|title| match reference.kind { + ReferenceKind::Chapter => Parent { + kind: "book", + title: title.clone(), + volume: reference.volume.clone(), + issue: None, + publisher: reference.publisher.clone(), + }, + _ => Parent { + kind: "periodical", + title: title.clone(), + volume: reference.volume.clone(), + issue: reference.issue.clone(), + publisher: None, + }, + }); + + Entry { + kind: hayagriva_kind(reference.kind), + title: reference.title.clone(), + author: reference.authors.clone(), + date: reference.year, + edition: reference.edition.clone(), + // The publisher belongs to the container when there is one. + publisher: if parent.is_some() { + None + } else { + reference.publisher.clone() + }, + page_range: reference.pages.clone(), + serial_number: serial, + url: reference.url.clone(), + parent, + } +} + +/// The Hayagriva entry type for a course reference kind. +fn hayagriva_kind(kind: ReferenceKind) -> &'static str { + match kind { + ReferenceKind::Book => "book", + ReferenceKind::Chapter => "chapter", + // Hayagriva has no preprint type. An article with no periodical parent + // is what a preprint is anyway. + ReferenceKind::Article | ReferenceKind::Preprint => "article", + ReferenceKind::Thesis => "thesis", + ReferenceKind::Website => "web", + ReferenceKind::Software | ReferenceKind::Dataset => "repository", + ReferenceKind::Video => "video", + ReferenceKind::Other => "misc", + } +} + +// --- CSL-JSON --- + +/// Renders the bibliography as CSL-JSON. +fn csl_json(course: &CourseFile) -> Result { + let items: Vec = course + .references + .iter() + .map(|(key, reference)| csl_item(key, reference)) + .collect(); + serde_json::to_string_pretty(&items) + .map(|text| format!("{text}\n")) + .map_err(crate::error::Error::other) +} + +/// One CSL-JSON item. +fn csl_item(key: &str, reference: &Reference) -> Value { + let mut item = json!({ + "id": key, + "type": csl_kind(reference.kind), + "title": reference.title, + }); + let map = item.as_object_mut().expect("built from a JSON object"); + + if !reference.authors.is_empty() { + map.insert( + "author".to_string(), + Value::Array(reference.authors.iter().map(|a| csl_name(a)).collect()), + ); + } + if let Some(year) = reference.year { + map.insert("issued".to_string(), json!({ "date-parts": [[year]] })); + } + for (field, value) in [ + ("container-title", &reference.container), + ("publisher", &reference.publisher), + ("volume", &reference.volume), + ("issue", &reference.issue), + ("page", &reference.pages), + ("edition", &reference.edition), + ("DOI", &reference.doi), + ("ISBN", &reference.isbn), + ("PMID", &reference.pmid), + ("PMCID", &reference.pmcid), + ("URL", &reference.url), + ] { + if let Some(value) = value { + map.insert(field.to_string(), Value::String(value.clone())); + } + } + item +} + +/// Splits `Family, Given` into a CSL name, or keeps it whole. +/// +/// A name with no comma is not a name this code can take apart — an +/// organization, or a single mononym — so it goes in `literal`, which is what +/// CSL has the field for. +fn csl_name(author: &str) -> Value { + match author.split_once(',') { + Some((family, given)) => json!({ + "family": family.trim(), + "given": given.trim(), + }), + None => json!({ "literal": author.trim() }), + } +} + +/// The CSL type for a course reference kind. +fn csl_kind(kind: ReferenceKind) -> &'static str { + match kind { + ReferenceKind::Book => "book", + ReferenceKind::Chapter => "chapter", + ReferenceKind::Article => "article-journal", + ReferenceKind::Preprint => "article", + ReferenceKind::Thesis => "thesis", + ReferenceKind::Website => "webpage", + ReferenceKind::Software => "software", + ReferenceKind::Dataset => "dataset", + ReferenceKind::Video => "motion_picture", + ReferenceKind::Other => "document", + } +} + +// --- BibTeX --- + +/// Renders the bibliography as BibTeX. +fn bibtex(course: &CourseFile) -> String { + let mut out = String::new(); + for (key, reference) in &course.references { + out.push_str(&bibtex_entry(key, reference)); + out.push('\n'); + } + out +} + +/// One BibTeX entry. +fn bibtex_entry(key: &str, reference: &Reference) -> String { + let mut fields: Vec<(&str, String)> = vec![("title", reference.title.clone())]; + if !reference.authors.is_empty() { + fields.push(("author", reference.authors.join(" and "))); + } + if let Some(year) = reference.year { + fields.push(("year", year.to_string())); + } + if let Some(container) = &reference.container { + let field = match reference.kind { + ReferenceKind::Chapter => "booktitle", + _ => "journal", + }; + fields.push((field, container.clone())); + } + for (field, value) in [ + ("volume", &reference.volume), + ("number", &reference.issue), + ("edition", &reference.edition), + ("publisher", &reference.publisher), + ("doi", &reference.doi), + ("isbn", &reference.isbn), + ("url", &reference.url), + ("note", &reference.note), + ] { + if let Some(value) = value { + fields.push((field, value.clone())); + } + } + if let Some(pages) = &reference.pages { + fields.push(("pages", en_dash(pages))); + } + + let body: String = fields + .iter() + .map(|(field, value)| format!(" {field} = {{{}}},\n", escape_tex(value))) + .collect(); + format!("@{}{{{key},\n{body}}}\n", bibtex_kind(reference.kind)) +} + +/// The BibTeX entry type for a course reference kind. +fn bibtex_kind(kind: ReferenceKind) -> &'static str { + match kind { + ReferenceKind::Book => "book", + ReferenceKind::Chapter => "incollection", + ReferenceKind::Article => "article", + ReferenceKind::Thesis => "phdthesis", + ReferenceKind::Website => "online", + // BibTeX proper has nothing for these. `misc` with a `note` is what + // every style guide says to do, and biblatex users can convert. + ReferenceKind::Preprint + | ReferenceKind::Software + | ReferenceKind::Dataset + | ReferenceKind::Video + | ReferenceKind::Other => "misc", + } +} + +/// Escapes the characters BibTeX treats as syntax. +/// +/// Deliberately short: a publisher called `John Wiley & Sons` is the case that +/// actually occurs, and escaping more than this risks mangling the `$...$` in a +/// title that carries real mathematics. +fn escape_tex(value: &str) -> String { + value.replace('&', "\\&").replace('%', "\\%") +} + +/// Turns a hyphenated page range into the en dash BibTeX expects. +fn en_dash(pages: &str) -> String { + let parts: Vec<&str> = pages.split('-').collect(); + if parts.len() == 2 && parts.iter().all(|p| !p.is_empty()) { + return format!("{}--{}", parts[0].trim(), parts[1].trim()); + } + pages.to_string() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::course::ReferenceRole; + + fn course() -> CourseFile { + let mut c = CourseFile::skeleton("BIOSC 1540", "Computational Biology", "2026f"); + c.references.insert( + "ismail2023bioinformatics".into(), + Reference { + label: Some("IBB".into()), + kind: ReferenceKind::Book, + role: ReferenceRole::Supplemental, + title: "Bioinformatics: A practical guide".into(), + authors: vec!["Ismail, H. D.".into()], + year: Some(2023), + publisher: Some("CRC Press".into()), + base_url: Some("https://library.example.org/ismail2023/".into()), + isbn: Some("9781032366423".into()), + ..Reference::default() + }, + ); + c.references.insert( + "altschul1990basic".into(), + Reference { + label: Some("BLAST".into()), + kind: ReferenceKind::Article, + role: ReferenceRole::Required, + title: "Basic local alignment search tool".into(), + authors: vec![ + "Altschul, S. F.".into(), + "Gish, W.".into(), + "Wiley & Sons".into(), + ], + year: Some(1990), + container: Some("Journal of Molecular Biology".into()), + volume: Some("215".into()), + issue: Some("3".into()), + pages: Some("403-410".into()), + doi: Some("10.1016/S0022-2836(05)80360-2".into()), + pmid: Some("2231712".into()), + ..Reference::default() + }, + ); + c + } + + #[test] + fn hayagriva_nests_an_article_under_its_periodical() { + let out = render(&course(), Format::Hayagriva).unwrap(); + assert!(out.contains("altschul1990basic:"), "{out}"); + assert!(out.contains("type: article"), "{out}"); + assert!(out.contains("type: periodical"), "{out}"); + assert!(out.contains("Journal of Molecular Biology"), "{out}"); + assert!(out.contains("page-range: 403-410"), "{out}"); + assert!(out.contains("doi: 10.1016/S0022-2836(05)80360-2"), "{out}"); + // Parses back as YAML, which is what Typst will do to it. + let back: serde_yaml_ng::Value = serde_yaml_ng::from_str(&out).unwrap(); + assert!(back.get("ismail2023bioinformatics").is_some()); + } + + #[test] + fn hayagriva_keeps_a_books_publisher_on_the_book() { + let out = render(&course(), Format::Hayagriva).unwrap(); + let parsed: serde_yaml_ng::Value = serde_yaml_ng::from_str(&out).unwrap(); + let book = parsed.get("ismail2023bioinformatics").unwrap(); + assert_eq!(book.get("publisher").unwrap().as_str(), Some("CRC Press")); + assert!(book.get("parent").is_none()); + // `label`, `role`, and `base_url` have nowhere to go and stay behind. + assert!(book.get("label").is_none()); + assert!(book.get("base_url").is_none()); + } + + #[test] + fn csl_json_splits_names_and_keeps_organizations_whole() { + let out = render(&course(), Format::CslJson).unwrap(); + let items: Vec = serde_json::from_str(&out).unwrap(); + let article = items + .iter() + .find(|i| i["id"] == "altschul1990basic") + .unwrap(); + assert_eq!(article["type"], "article-journal"); + assert_eq!(article["author"][0]["family"], "Altschul"); + assert_eq!(article["author"][0]["given"], "S. F."); + assert_eq!(article["author"][2]["literal"], "Wiley & Sons"); + assert_eq!(article["issued"]["date-parts"][0][0], 1990); + assert_eq!(article["DOI"], "10.1016/S0022-2836(05)80360-2"); + } + + #[test] + fn bibtex_escapes_ampersands_and_dashes_a_page_range() { + let out = render(&course(), Format::Bibtex).unwrap(); + assert!(out.contains("@article{altschul1990basic,"), "{out}"); + assert!( + out.contains("journal = {Journal of Molecular Biology},"), + "{out}" + ); + assert!(out.contains("pages = {403--410},"), "{out}"); + assert!(out.contains("Wiley \\& Sons"), "{out}"); + assert!(out.contains("@book{ismail2023bioinformatics,"), "{out}"); + } + + #[test] + fn a_course_with_no_bibliography_renders_empty_rather_than_failing() { + let mut c = course(); + c.references.clear(); + assert_eq!(render(&c, Format::Bibtex).unwrap(), ""); + assert_eq!(render(&c, Format::CslJson).unwrap().trim(), "[]"); + } +} diff --git a/src/export/site.rs b/src/export/site.rs index 669889d..d1e23fb 100644 --- a/src/export/site.rs +++ b/src/export/site.rs @@ -34,7 +34,7 @@ use crate::assessment::{AssessmentFile, Form, Placement}; use crate::catalog::Catalog; -use crate::course::{CourseFile, Reference}; +use crate::course::CourseFile; use crate::error::{Error, Result}; use crate::item::{Choice, Citation, Item, Solution}; use crate::markup; @@ -472,7 +472,7 @@ fn cite_html(course: &CourseFile, citation: &Citation) -> String { let Some(reference) = course.references.get(key) else { return markup::escape_html(&citation.display()); }; - let label = reference.label.as_deref().unwrap_or(key); + let label = reference.label_or(key); let locator = citation.locator.as_deref().unwrap_or(""); let body = if locator.is_empty() { markup::escape_html(label) @@ -483,27 +483,12 @@ fn cite_html(course: &CourseFile, citation: &Citation) -> String { markup::escape_html(locator) ) }; - match resolve_url(citation, reference) { + match citation.href(reference) { Some(url) => format!("{body}", markup::escape_html(&url)), None => body, } } -/// Resolves a citation's link, from an explicit URL or a path joined to the -/// reference's base URL. -fn resolve_url(citation: &Citation, reference: &Reference) -> Option { - if let Some(url) = &citation.url { - return Some(url.clone()); - } - let path = citation.path.as_deref()?; - let base = reference.base_url.as_deref()?; - Some(match (base.ends_with('/'), path.starts_with('/')) { - (true, true) => format!("{base}{}", &path[1..]), - (false, false) => format!("{base}/{path}"), - _ => format!("{base}{path}"), - }) -} - // --- math-aware markup --- /// One run of source text, split on math delimiters. @@ -858,6 +843,7 @@ items: number: 1, item: "b::q-mcq".into(), version: None, + stem_digest: None, fingerprint: None, points: Some(1.0), bonus: false, @@ -873,6 +859,7 @@ items: number: 2, item: "b::q-open".into(), version: None, + stem_digest: None, fingerprint: None, points: Some(2.0), bonus: false, diff --git a/src/export/typst.rs b/src/export/typst.rs index a1762a9..641e70c 100644 --- a/src/export/typst.rs +++ b/src/export/typst.rs @@ -431,6 +431,7 @@ mod tests { number: 1, item: "b::q-1".into(), version: None, + stem_digest: None, fingerprint: None, points: None, bonus: false, @@ -446,6 +447,7 @@ mod tests { number: 2, item: "b::q-2".into(), version: None, + stem_digest: None, fingerprint: None, points: None, bonus: false, @@ -464,6 +466,6 @@ mod tests { .filter(|p| p.was_printed()) .map(|p| p.number) .collect(); - assert_eq!(printable, vec![2]); + assert_eq!(printable, vec![1, 2]); } } diff --git a/src/export/typst/diagnostic.rs b/src/export/typst/diagnostic.rs index 850aee5..ea05931 100644 --- a/src/export/typst/diagnostic.rs +++ b/src/export/typst/diagnostic.rs @@ -1281,6 +1281,7 @@ mod tests { blueprint: Vec::new(), patterns: Vec::new(), warnings: Vec::new(), + dropped_detail: Vec::new(), } } diff --git a/src/lib.rs b/src/lib.rs index d5559ea..d9d6294 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,12 +18,19 @@ //! //! | File | Holds | Written by | //! |:--|:--|:--| -//! | `course.yaml` | identity, policy, objectives, lectures | you | +//! | `course.yaml` | identity, policy, units | you | +//! | `references.yaml` | the works the course cites | you | +//! | `lectures/*.yaml` | one lecture: readings, and what it teaches | you | +//! | `objectives/*.yaml` | one objective and its learning targets | you | //! | `banks/*.yaml` | items, with design intent and pooled statistics | you, then `calibrate` | //! | `assessments/*.yaml` | what was given, to whom, when | `assemble`, then you | //! | `data/*.parquet` | one row per student per item | `ingest` | //! -//! Three of the four are hand-editable YAML meant to be reviewed in a pull request. +//! The first four are one course file split by subject; `coursebank course build` +//! prints the merged result, and a course that keeps everything in `course.yaml` +//! still loads unchanged. See [`course::fragment`]. +//! +//! Most of these are hand-editable YAML meant to be reviewed in a pull request. //! Only the response data is machine-only, and it is stored in an open columnar //! format so pandas, polars, R, and DuckDB can all read it without this tool. //! @@ -95,6 +102,7 @@ pub mod data; pub mod error; pub mod export; pub mod guide; +pub mod migrate; pub mod model; pub mod util; @@ -102,6 +110,8 @@ pub use util::{date, hash, markup, rng, yaml, zipfile}; pub use model::{assessment, bank, catalog, course, history, item, layout, seal, taxonomy}; +pub use course::fragment; + pub use authoring::{jsonschema, lint, select}; pub use data::store_parquet; @@ -110,7 +120,7 @@ pub use data::{canvas, decode, gradescope, intake, responses, store}; pub use analysis::{calibrate, classical, diagnostic, irt, students}; pub use export::site; -pub use export::{lecture, practice, qti, report, typst}; +pub use export::{lecture, practice, qti, references, report, typst}; pub use catalog::Catalog; pub use course::{CourseFile, SCHEMA_VERSION}; diff --git a/src/migrate.rs b/src/migrate.rs new file mode 100644 index 0000000..bfe7d0c --- /dev/null +++ b/src/migrate.rs @@ -0,0 +1,1896 @@ +// SPDX-License-Identifier: Prosperity-3.0.0 +// Copyright Scientific Computing Studio +// Source: https://git.scient.ing/education/coursebank + +//! One-time conversions from one layout to the next. +//! +//! Everything here is a command you run once and then delete from your shell +//! history. It lives in the binary anyway, because the alternative is a script +//! in a gist that nobody can find in two years when a colleague clones the +//! course. +//! +//! # What there is +//! +//! | Function | Rewrites | Because | +//! |:--|:--|:--| +//! | [`split_plan`] | `course.yaml` into fragments | one file per lecture beats one file | +//! | [`qualified_ids`] | `item:` in records and seals | an id names the item, not its bank | +//! | [`store_ids`] | `item_ref` in the response store | so other readers see one id per item | +//! | [`options_plan`] and [`apply_options`] | option letters into names | a letter is a position, not an identity | +//! | [`stems`] | drops `version:` and `history:` | a stem's text is its identity | +//! +//! # What the option migration does not touch +//! +//! Seals. A seal's digest covers the option ids it froze, so renaming them +//! would either invalidate the digest or require recomputing it — and a +//! tamper-evident record of an administration that has been quietly recomputed +//! is worth less than one that plainly says it predates the rename. A seal +//! keeps its letters, and [`crate::decode`] reads it as it stands. +//! +//! # Splitting the course file +//! +//! [`split_plan`] turns one `course.yaml` into a directory of fragments: the +//! bibliography into `references.yaml`, each lecture into `lectures/l-1-2.yaml`, +//! and each objective with its targets into `objectives/lo-....yaml`. +//! [`fragment::assemble`](crate::course::fragment::assemble) merges them back. +//! +//! ## Why this works on text +//! +//! The obvious implementation parses the course file, partitions the model, and +//! serializes each part. It is also the wrong one. A hand-written course file +//! carries section comments, folded block scalars, and deliberate quoting, and +//! `serde` round-tripping discards all three: five hundred lines of reading +//! prose come back as reflowed one-line scalars, and `# === L1.2` is simply +//! gone. The diff would be unreviewable, which for a migration is the whole +//! game. +//! +//! So the split copies *lines*. Fragments use the same section keys at the same +//! nesting depth as the file they came from, which means moving a lecture out is +//! a verbatim copy with no re-indentation, and every comment and scalar style +//! survives. Two edits are made to the copied text, both line-oriented: a +//! `teaches:` list is inserted into each lecture, and the `lectures:` lines that +//! [`fragment::assemble`](crate::course::fragment::assemble) now derives are +//! dropped. +//! +//! The scanner underneath is not a YAML parser and does not need to be. It finds +//! keys at an exact indentation, and a line at that indentation cannot be inside +//! a block scalar, because scalar content is indented deeper than the key that +//! introduces it. What it produces is checked the only way worth checking: the +//! written fragments are loaded back and compared against the original model by +//! [`differences`]. + +use std::collections::{BTreeMap, BTreeSet}; +use std::path::{Path, PathBuf}; + +use crate::course::fragment::REFERENCES_FILE; +use crate::course::{COURSE_FILE, CourseFile}; +use crate::error::{Error, Result}; +use crate::item::{Choice, Item}; +use crate::layout::Layout; +use crate::store::{self, Store}; +use crate::yaml; + +/// The width past which a flow sequence is written as a block list instead. +const FLOW_WIDTH: usize = 96; + +/// One file a split would write. +#[derive(Debug, Clone)] +pub struct PlannedFile { + /// Where it goes, relative to the course root. + pub path: PathBuf, + /// Its full contents. + pub text: String, + /// What it holds, for the command's output. + pub summary: String, +} + +/// What a split would do. +#[derive(Debug, Clone, Default)] +pub struct Plan { + /// The files to write, `course.yaml` first. + pub files: Vec, + /// Things worth saying out loud before writing. + pub notes: Vec, +} + +impl Plan { + /// The number of lines each planned file holds. + pub fn lines(&self) -> Vec<(PathBuf, usize)> { + self.files + .iter() + .map(|f| (f.path.clone(), f.text.lines().count())) + .collect() + } +} + +/// Plans the split of a course directory's `course.yaml`. +/// +/// Nothing is written. The plan holds complete file contents, so the caller can +/// print it, write it, or throw it away. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// +/// # Returns +/// +/// The files a split would write. +/// +/// # Errors +/// +/// Returns [`Error::Io`] or [`Error::Yaml`] if `course.yaml` cannot be read or +/// parsed, and [`Error::Usage`] when the course is already split. +pub fn split_plan(root: &Path) -> Result { + let layout = Layout::new(root); + let course_path = layout.course_file(); + let course = CourseFile::load(&course_path)?; + + for dir in [layout.lectures(), layout.objectives()] { + if !yaml::list_yaml(&dir)?.is_empty() { + return Err(Error::usage(format!( + "{} already holds YAML files, so this course is already split", + dir.display() + ))); + } + } + if layout.references_file().exists() { + return Err(Error::usage(format!( + "{} already exists", + layout.references_file().display() + ))); + } + + let text = std::fs::read_to_string(&course_path).map_err(|e| Error::io(&course_path, e))?; + let lines: Vec = text.lines().map(str::to_string).collect(); + let sections = blocks(&lines, 0); + + let mut plan = Plan::default(); + + // Which objectives each lecture develops, and which targets each objective + // decomposes into. Both come from the parsed model rather than the text, + // because both are relations rather than syntax. + let mut teaches: BTreeMap<&str, Vec<&str>> = BTreeMap::new(); + for (id, objective) in &course.learning_objectives { + for lecture in &objective.lectures { + teaches + .entry(lecture.as_str()) + .or_default() + .push(id.as_str()); + } + } + let mut targets_of: BTreeMap<&str, Vec<&str>> = BTreeMap::new(); + for (id, target) in &course.learning_targets { + targets_of + .entry(target.objective.as_str()) + .or_default() + .push(id.as_str()); + } + + // 1. The root keeps identity, policy, units, and anything not moved. + let moved = [ + "references", + "lectures", + "learning_objectives", + "learning_targets", + ]; + let mut root_text = String::new(); + for section in §ions { + if moved.contains(§ion.key.as_str()) { + continue; + } + root_text.push_str(§ion.text()); + root_text.push_str("\n\n"); + } + plan.files.push(PlannedFile { + path: PathBuf::from(COURSE_FILE), + text: tidy(&root_text), + summary: "identity, policy, units".to_string(), + }); + + // 2. The bibliography. + if let Some(section) = sections.iter().find(|s| s.key == "references") { + plan.files.push(PlannedFile { + path: PathBuf::from(REFERENCES_FILE), + text: tidy(§ion.text()), + summary: format!("{} reference(s)", course.references.len()), + }); + } + + // 3. One file per lecture, with a `teaches:` list inserted. + if let Some(section) = sections.iter().find(|s| s.key == "lectures") { + for block in blocks(§ion.body(), 2) { + let id = block.key.clone(); + let listed = teaches.get(id.as_str()).cloned().unwrap_or_default(); + let mut body = block.lines.clone(); + if !listed.is_empty() { + body = with_teaches(&body, &listed); + } + let readings = course + .lectures + .get(&id) + .map(|l| l.readings.len()) + .unwrap_or(0); + plan.files.push(PlannedFile { + path: layout_relative("lectures", &lecture_slug(&id)), + text: tidy(&format!( + "lectures:\n{}\n", + join(&strip_group_header(&block.lead, &[id.as_str()]), &body) + )), + summary: format!("{id}: {} objective(s), {readings} reading(s)", listed.len()), + }); + } + } + + // 4. One file per objective, holding its targets. + let objective_blocks: BTreeMap = sections + .iter() + .find(|s| s.key == "learning_objectives") + .map(|s| { + blocks(&s.body(), 2) + .into_iter() + .map(|b| (b.key.clone(), b)) + .collect() + }) + .unwrap_or_default(); + let target_blocks: Vec = sections + .iter() + .find(|s| s.key == "learning_targets") + .map(|s| blocks(&s.body(), 2)) + .unwrap_or_default(); + + for (id, objective) in &course.learning_objectives { + let Some(block) = objective_blocks.get(id) else { + plan.notes.push(format!( + "objective `{id}` was not found in the text; skipped" + )); + continue; + }; + let lecture_labels: Vec<&str> = objective.lectures.iter().map(String::as_str).collect(); + let mut labels = lecture_labels.clone(); + labels.push(id.as_str()); + + // `lectures:` on the objective is derived from each lecture's `teaches:`, + // so carrying it here as well would be the duplication this move exists + // to remove. + let body = without_key(&block.lines, 4, "lectures"); + let mut text = format!( + "learning_objectives:\n{}\n", + join(&strip_group_header(&block.lead, &labels), &body) + ); + + // Taken from the text in the order it was written, not from the model in + // id order: a target's `order:` follows the sequence it was authored in, + // and alphabetizing 165 of them would scramble every file. + let mine: Vec<&Block> = target_blocks + .iter() + .filter(|b| { + course + .learning_targets + .get(&b.key) + .is_some_and(|t| t.objective == *id) + }) + .collect(); + + if !mine.is_empty() { + text.push_str("\nlearning_targets:\n"); + let objective_lectures: BTreeSet<&str> = + objective.lectures.iter().map(String::as_str).collect(); + for target_block in &mine { + let target = &course.learning_targets[target_block.key.as_str()]; + let own: BTreeSet<&str> = target.lectures.iter().map(String::as_str).collect(); + // A target inherits its objective's lectures, so an identical + // list is noise. A narrower one is a real claim and stays. + let body = if own == objective_lectures { + without_key(&target_block.lines, 4, "lectures") + } else { + target_block.lines.clone() + }; + text.push_str(&join( + &strip_group_header(&target_block.lead, &[id.as_str()]), + &body, + )); + text.push('\n'); + } + } + + let expected = targets_of.get(id.as_str()).map(Vec::len).unwrap_or(0); + if mine.len() != expected { + plan.notes.push(format!( + "objective `{id}`: {} of its {expected} target(s) were found in the text", + mine.len() + )); + } + + plan.files.push(PlannedFile { + path: layout_relative("objectives", id), + text: tidy(&text), + summary: format!("{id}: {} target(s)", mine.len()), + }); + } + + if !course.stimuli.is_empty() { + plan.notes.push(format!( + "{} stimulus/stimuli left in {COURSE_FILE}: which objective owns one is an \ + item-level question, so moving them waits for the per-objective banks", + course.stimuli.len() + )); + } + plan.notes.push( + "comments between two entries are attached to the entry below them, which is where a \ + `# === ...` group header belongs. Check any comment that trailed an entry." + .to_string(), + ); + + Ok(plan) +} + +/// Writes a plan, backing up the file it replaces. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `plan` - the plan to write. +/// +/// # Returns +/// +/// The paths written, plus the backup. +/// +/// # Errors +/// +/// Returns [`Error::Usage`] if a backup already exists, or [`Error::Io`] on a +/// write failure. +pub fn apply(root: &Path, plan: &Plan) -> Result> { + let backup = root.join(format!("{COURSE_FILE}.bak")); + if backup.exists() { + return Err(Error::usage(format!( + "{} already exists; move it aside first so a second run cannot overwrite the only \ + copy of the original", + backup.display() + ))); + } + std::fs::copy(root.join(COURSE_FILE), &backup).map_err(|e| Error::io(&backup, e))?; + + let mut written = vec![backup]; + for file in &plan.files { + let path = root.join(&file.path); + yaml::write_text(&path, &file.text)?; + written.push(path); + } + Ok(written) +} + +/// Compares two courses for everything a split is supposed to preserve. +/// +/// Lecture lists are compared as sets, since a derived list is ordered by the +/// files it was derived from, and `teaches` is ignored on the way in because +/// nothing declared it before the split. +/// +/// # Arguments +/// +/// * `before` - the course as it was. +/// * `after` - the course as reassembled from fragments. +/// +/// # Returns +/// +/// One message per difference, empty when the two agree. +/// +/// # Errors +/// +/// Returns [`Error::Other`] if either model cannot be serialized. +pub fn differences(before: &CourseFile, after: &CourseFile) -> Result> { + let old = projection(before)?; + let new = projection(after)?; + let mut out = Vec::new(); + + for (key, value) in &old { + match new.get(key) { + None => out.push(format!("{key} was lost")), + Some(other) if other != value => out.push(format!("{key} changed")), + Some(_) => {} + } + } + for key in new.keys() { + if !old.contains_key(key) { + out.push(format!("{key} appeared")); + } + } + Ok(out) +} + +/// A comparable, order-insensitive view of a course. +fn projection(course: &CourseFile) -> Result> { + let mut out = BTreeMap::new(); + out.insert("course".to_string(), yaml::to_string(&course.course)?); + out.insert("policy".to_string(), yaml::to_string(&course.policy)?); + out.insert("units".to_string(), yaml::to_string(&course.units)?); + out.insert("stimuli".to_string(), yaml::to_string(&course.stimuli)?); + + for (id, reference) in &course.references { + out.insert(format!("reference `{id}`"), yaml::to_string(reference)?); + } + for (id, lecture) in &course.lectures { + let mut lecture = lecture.clone(); + lecture.teaches.clear(); + out.insert(format!("lecture `{id}`"), yaml::to_string(&lecture)?); + } + for (id, objective) in &course.learning_objectives { + let mut objective = objective.clone(); + objective.lectures.sort(); + out.insert(format!("objective `{id}`"), yaml::to_string(&objective)?); + } + for (id, target) in &course.learning_targets { + let mut target = target.clone(); + target.lectures.sort(); + out.insert(format!("target `{id}`"), yaml::to_string(&target)?); + } + Ok(out) +} + +/// A file name for one fragment, relative to the course root. +fn layout_relative(dir: &str, stem: &str) -> PathBuf { + PathBuf::from(dir).join(format!("{stem}.yaml")) +} + +/// The file stem for a lecture id: `L1.2` becomes `l-1-2`. +/// +/// Matches how the bank and assessment files are already named, so a directory +/// listing sorts the way the course runs. +fn lecture_slug(id: &str) -> String { + let mut out = String::new(); + let mut previous: Option = None; + for ch in id.chars() { + if ch.is_ascii_alphanumeric() { + let boundary = + matches!(previous, Some(p) if p.is_ascii_alphabetic() != ch.is_ascii_alphabetic()); + if boundary && !out.ends_with('-') { + out.push('-'); + } + out.extend(ch.to_lowercase()); + } else if !out.is_empty() && !out.ends_with('-') { + out.push('-'); + } + previous = Some(ch); + } + out.trim_matches('-').to_string() +} + +/// Rewrites pre-2.0 bank-qualified item ids in the files that carry them. +/// +/// Textual, for the same reason [`split_plan`] is: an assessment record is +/// hand-edited and carries comments, and a serde round trip would drop them to +/// change one field. Only the value of an `item:` key is touched, so a +/// fingerprint or a note that happens to contain `::` is left alone. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// +/// # Returns +/// +/// One entry per file that would change, with how many ids it holds. +/// +/// # Errors +/// +/// Returns [`Error::Io`] when a directory or file cannot be read. +pub fn qualified_ids(root: &Path) -> Result> { + let layout = Layout::new(root); + let mut out = Vec::new(); + + for dir in [layout.assessments(), layout.seals()] { + for path in yaml::list_yaml(&dir)? { + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let rewritten = unqualify(&text); + if rewritten != text { + let n = text + .lines() + .zip(rewritten.lines()) + .filter(|(a, b)| a != b) + .count(); + let shown = path.strip_prefix(root).unwrap_or(&path).to_path_buf(); + out.push((shown, n, rewritten)); + } + } + } + Ok(out) +} + +/// Writes the rewritten files a call to [`qualified_ids`] produced. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `changes` - the plan. +/// +/// # Returns +/// +/// The paths written. +/// +/// # Errors +/// +/// Returns [`Error::Io`] on a write failure. +pub fn apply_ids(root: &Path, changes: &[(PathBuf, usize, String)]) -> Result> { + let mut written = Vec::new(); + for (relative, _, text) in changes { + let path = root.join(relative); + yaml::write_text(&path, text)?; + written.push(path); + } + Ok(written) +} + +/// Strips the bank qualifier from every `item:` value in a YAML document. +fn unqualify(text: &str) -> String { + let mut out: Vec = Vec::new(); + for line in text.lines() { + out.push(unqualify_line(line)); + } + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + joined +} + +/// Strips the bank qualifier from an `item:` value on one line. +/// +/// Handles both the block form and the flow form the records use: +/// +/// ```text +/// item: b-1-2::q-fastq-line -> item: q-fastq-line +/// { number: 1, item: "b1::q-a-001" } -> { number: 1, item: "q-a-001" } +/// ``` +fn unqualify_line(line: &str) -> String { + let Some(at) = line.find("item:") else { + return line.to_string(); + }; + // `learning_targets:` and `dropped_before_printing:` do not end in `item:`, + // but `item:` must be a whole key rather than a suffix of one. + let before = &line[..at]; + if before + .chars() + .next_back() + .is_some_and(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return line.to_string(); + } + + let rest = &line[at + "item:".len()..]; + let value_at = rest.len() - rest.trim_start().len(); + let value = rest.trim_start(); + let quote = value.starts_with(['"', '\'']); + let inner = if quote { &value[1..] } else { value }; + let end = inner + .find(|c: char| c == '"' || c == '\'' || c == ',' || c == '}' || c.is_whitespace()) + .unwrap_or(inner.len()); + let (id, tail) = inner.split_at(end); + match id.split_once(crate::item::LEGACY_QUALIFIER) { + None => line.to_string(), + Some((_, bare)) => format!( + "{}{}{}{}{}", + &line[..at + "item:".len()], + &rest[..value_at], + if quote { &value[..1] } else { "" }, + bare, + tail + ), + } +} + +/// Rewrites the item ids in the response store. +/// +/// The reader already canonicalizes ids in memory, so nothing depends on this +/// having been run. What it is for is the other readers: the store is Parquet +/// precisely so that pandas, DuckDB, and R can use it without this tool, and +/// those readers see whatever is actually in the column. A store holding both +/// `b-1-2::q-x` and `q-x` groups one question into two. +/// +/// Rows are rewritten flat, without a round trip through +/// [`crate::responses::Response`], so a column this migration is not about +/// cannot be reshaped on the way through. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `write` - whether to write, or only to count. +/// +/// # Returns +/// +/// One entry per file holding qualified ids, with how many rows carry them. +/// +/// # Errors +/// +/// Propagates read and write failures, including +/// [`Error::FeatureDisabled`] when the store is Parquet and the feature is off. +pub fn store_ids(root: &Path, write: bool) -> Result> { + let layout = Layout::new(root); + let store = Store::open(layout.data())?; + let mut out = Vec::new(); + + for path in store.files()? { + let mut rows = store::read_flat(&path)?; + let mut touched = 0; + for row in &mut rows { + let canonical = crate::item::canonical_id(&row.item_ref); + if canonical != row.item_ref { + row.item_ref = canonical.to_string(); + touched += 1; + } + } + if touched == 0 { + continue; + } + if write { + store::write_flat(&path, &rows)?; + } + let shown = path.strip_prefix(root).unwrap_or(&path).to_path_buf(); + out.push((shown, touched)); + } + Ok(out) +} + +/// The per-item map from a pre-2.0 option letter to the name that replaces it. +pub type OptionMap = BTreeMap>; + +/// Words an option id gains nothing from carrying. +const FUNCTION_WORDS: [&str; 40] = [ + "the", "a", "an", "is", "are", "was", "were", "of", "to", "for", "from", "in", "on", "at", + "and", "or", "but", "while", "that", "which", "it", "its", "they", "their", "this", "these", + "those", "be", "been", "with", "by", "as", "so", "than", "then", "each", "every", "one", + "about", "into", +]; + +/// How many words an option id takes from the option's text. +const SLUG_WORDS: usize = 5; + +/// How many characters of words an option id takes. +const SLUG_BUDGET: usize = 34; + +/// Derives an option id for every option of one item. +/// +/// The ids have to be readable, because they end up in a student report's +/// diagnostics, in `credit_overrides`, and in the `selected` column of every +/// response row — and they have to be unique within the item, because that is +/// what makes them an identity at all. +/// +/// Leading words give a readable id for most options. Where two of them come +/// out the same, each is extended with the first word that tells it apart from +/// the others it collided with, preferring a content word: three options that +/// all begin "its error probability falls to about one" end in `-half`, +/// `-tenth`, and `-hundredth`. +/// +/// # Arguments +/// +/// * `texts` - the option texts, in the order the item declares them. +/// +/// # Returns +/// +/// One id per option, in the same order. Empty when two options are so alike +/// that no word distinguishes them, which is a signal to look at the item +/// rather than to suffix a number onto it. +pub fn option_ids(texts: &[String]) -> Vec { + let words: Vec> = texts.iter().map(|t| slug_words(t)).collect(); + let mut stems: Vec = words.iter().map(|w| stem(w)).collect(); + + // Groups are taken before anything is extended: extending as we go would + // leave the last member of a group looking like the bare prefix its + // siblings were derived from. + let mut groups: BTreeMap> = BTreeMap::new(); + for (i, s) in stems.iter().enumerate() { + groups.entry(s.clone()).or_default().push(i); + } + for (_, group) in groups.iter().filter(|(_, g)| g.len() > 1) { + for &i in group { + let others: Vec<&Vec> = group + .iter() + .filter(|&&j| j != i) + .map(|&j| &words[j]) + .collect(); + if let Some(word) = distinguishing(&words[i], &others) { + stems[i] = format!("{}-{word}", stems[i]); + } + } + } + + if stems.iter().collect::>().len() != stems.len() { + return Vec::new(); + } + stems.iter().map(|s| format!("o-{s}")).collect() +} + +/// The words of an option's text, with markup and math flattened. +/// +/// Math is kept rather than stripped: two options that read `score $5$` and +/// `score $-1$` differ only inside the delimiters, and dropping them would make +/// the two ids identical. +fn slug_words(text: &str) -> Vec { + let mut flat = String::with_capacity(text.len()); + let mut in_math = false; + for ch in text.chars() { + match ch { + '$' => { + in_math = !in_math; + flat.push(' '); + } + '-' if in_math => flat.push_str(" minus "), + '`' | '*' | '_' => flat.push(' '), + c => flat.push(c.to_ascii_lowercase()), + } + } + flat.split(|c: char| !c.is_ascii_alphanumeric()) + .filter(|w| !w.is_empty()) + .map(str::to_string) + .collect() +} + +/// The leading words of an option, within the word and character budgets. +fn stem(words: &[String]) -> String { + let mut kept: Vec<&String> = Vec::new(); + let mut total = 0; + for word in words + .iter() + .skip_while(|w| matches!(w.as_str(), "the" | "a" | "an")) + { + if kept.len() == SLUG_WORDS || (total + word.len() > SLUG_BUDGET && !kept.is_empty()) { + break; + } + total += word.len() + 1; + kept.push(word); + } + // An id ending on `for` or `the` reads like a truncation, which it is. + while kept.len() > 1 && FUNCTION_WORDS.contains(&kept[kept.len() - 1].as_str()) { + kept.pop(); + } + if kept.is_empty() { + return "option".to_string(); + } + kept.iter() + .map(|w| w.as_str()) + .collect::>() + .join("-") +} + +/// The first word of `mine` that appears at that position in none of `others`. +fn distinguishing(mine: &[String], others: &[&Vec]) -> Option { + let differs = |at: usize, word: &str| { + others + .iter() + .all(|other| other.get(at).map(String::as_str) != Some(word)) + }; + mine.iter() + .enumerate() + .find(|(at, word)| !FUNCTION_WORDS.contains(&word.as_str()) && differs(*at, word)) + .or_else(|| { + mine.iter() + .enumerate() + .find(|(at, word)| differs(*at, word)) + }) + .map(|(_, word)| word.clone()) +} + +/// Derives the option ids for every item in every bank. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// +/// # Returns +/// +/// The map, and one message per item whose options could not be told apart. +/// +/// # Errors +/// +/// Propagates bank load failures. +pub fn options_plan(root: &Path) -> Result<(OptionMap, Vec)> { + let layout = Layout::new(root); + let mut map: OptionMap = BTreeMap::new(); + let mut problems = Vec::new(); + + for path in yaml::list_yaml(&layout.banks())? { + let bank: crate::bank::BankFile = yaml::read(&path)?; + for item in &bank.items { + let legacy: Vec<&Choice> = item + .options + .iter() + .filter(|o| Item::is_legacy_option_id(&o.id)) + .collect(); + if legacy.is_empty() { + continue; + } + if legacy.len() != item.options.len() { + problems.push(format!( + "{}: `{}` has both named and lettered options; name the rest by hand", + path.display(), + item.id + )); + continue; + } + let texts: Vec = item.options.iter().map(|o| o.text.clone()).collect(); + let ids = option_ids(&texts); + if ids.is_empty() { + problems.push(format!( + "{}: `{}` has options no word tells apart, so no id can be derived. Name \\ + them by hand, or look again at whether they are two readings of one option.", + path.display(), + item.id + )); + continue; + } + let entry: BTreeMap = + item.options.iter().map(|o| o.id.clone()).zip(ids).collect(); + map.insert(item.id.clone(), entry); + } + } + Ok((map, problems)) +} + +/// Rewrites option letters as names, in every file that carries one. +/// +/// Banks and assessment records are rewritten textually, for the reason +/// [`split_plan`] is: both are hand-edited and full of prose. The response +/// store is rewritten flat. Seals are *not* rewritten — see the note in the +/// module documentation. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `map` - the plan from [`options_plan`]. +/// * `write` - whether to write, or only to count. +/// +/// # Returns +/// +/// One entry per file that changes, with how many substitutions it holds. +/// +/// # Errors +/// +/// Propagates read and write failures. +pub fn apply_options(root: &Path, map: &OptionMap, write: bool) -> Result> { + let layout = Layout::new(root); + let mut out = Vec::new(); + + for path in yaml::list_yaml(&layout.banks())? { + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let (rewritten, n) = rename_in_bank(&text, map); + if n > 0 { + if write { + yaml::write_text(&path, &rewritten)?; + } + out.push((relative(root, &path), n)); + } + } + + for path in yaml::list_yaml(&layout.assessments())? { + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let (rewritten, n) = rename_in_record(&text, map); + if n > 0 { + if write { + yaml::write_text(&path, &rewritten)?; + } + out.push((relative(root, &path), n)); + } + } + + let store = Store::open(layout.data())?; + for path in store.files()? { + let mut rows = store::read_flat(&path)?; + let mut touched = 0; + for row in &mut rows { + let Some(item) = map.get(crate::item::canonical_id(&row.item_ref)) else { + continue; + }; + for column in [&mut row.selected, &mut row.eliminated] { + let renamed = rename_list(column, item); + if renamed != *column { + *column = renamed; + touched += 1; + } + } + } + if touched > 0 { + if write { + store::write_flat(&path, &rows)?; + } + out.push((relative(root, &path), touched)); + } + } + + Ok(out) +} + +/// A path shown relative to the course root. +fn relative(root: &Path, path: &Path) -> PathBuf { + path.strip_prefix(root).unwrap_or(path).to_path_buf() +} + +/// Renames the semicolon-joined option ids a stored response column holds. +fn rename_list(column: &str, item: &BTreeMap) -> String { + column + .split(';') + .map(|part| match item.get(part.trim()) { + Some(renamed) => renamed.as_str(), + None => part, + }) + .collect::>() + .join(";") +} + +/// Rewrites the `- id:` lines of every option in a bank file. +/// +/// Which `- id:` lines those are is decided by the plan rather than by +/// indentation: a value that names an item in the plan sets the item, and a +/// value the current item maps is an option of it. So a bank indented +/// unusually still migrates, and nothing else called `id` can be caught by +/// accident. +fn rename_in_bank(text: &str, map: &OptionMap) -> (String, usize) { + let mut out: Vec = Vec::new(); + let mut current: Option<&BTreeMap> = None; + let mut n = 0; + + for line in text.lines() { + let value = list_id(line); + if let Some(value) = value { + if let Some(item) = map.get(value) { + current = Some(item); + out.push(line.to_string()); + continue; + } + if let Some(renamed) = current.and_then(|item| item.get(value)) { + out.push(line.replace(value, renamed)); + n += 1; + continue; + } + } + out.push(line.to_string()); + } + + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + (joined, n) +} + +/// The value of a `- id: value` line, if the line is one. +fn list_id(line: &str) -> Option<&str> { + let trimmed = line.trim_start(); + let rest = trimmed.strip_prefix("- id:")?; + let value = rest.trim().trim_matches(['\'', '"']); + if value.is_empty() { None } else { Some(value) } +} + +/// Rewrites `key:` and `credit_overrides:` in an assessment record. +/// +/// Both belong to the placement whose `item:` was seen most recently, which is +/// how the records are written: the item and its key sit in one block, or on +/// one line. +fn rename_in_record(text: &str, map: &OptionMap) -> (String, usize) { + let mut out: Vec = Vec::new(); + let mut current: Option<&BTreeMap> = None; + let mut n = 0; + + for line in text.lines() { + if let Some(item) = item_on_line(line) { + current = map.get(crate::item::canonical_id(item)); + } + let Some(item) = current else { + out.push(line.to_string()); + continue; + }; + let (rewritten, count) = rename_bracketed(line, "key:", item); + let (rewritten, more) = rename_braced(&rewritten, "credit_overrides:", item); + n += count + more; + out.push(rewritten); + } + + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + (joined, n) +} + +/// The `item:` value on a line, if it carries one. +fn item_on_line(line: &str) -> Option<&str> { + let at = line.find("item:")?; + let before = &line[..at]; + if before + .chars() + .next_back() + .is_some_and(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return None; + } + let rest = line[at + "item:".len()..].trim_start(); + let quote = rest.starts_with(['"', '\'']); + let inner = if quote { &rest[1..] } else { rest }; + let end = inner + .find(|c: char| c == '"' || c == '\'' || c == ',' || c == '}' || c.is_whitespace()) + .unwrap_or(inner.len()); + Some(&inner[..end]).filter(|v| !v.is_empty()) +} + +/// Renames the members of a `key: [A, B]` list. +fn rename_bracketed(line: &str, key: &str, item: &BTreeMap) -> (String, usize) { + let Some(at) = key_at_word(line, key) else { + return (line.to_string(), 0); + }; + let rest = &line[at + key.len()..]; + let Some(open) = rest.find('[') else { + return (line.to_string(), 0); + }; + let Some(close) = rest[open..].find(']') else { + return (line.to_string(), 0); + }; + let inside = &rest[open + 1..open + close]; + let mut n = 0; + let renamed: Vec = inside + .split(',') + .map(|part| { + let trimmed = part.trim().trim_matches(['\'', '"']); + match item.get(trimmed) { + Some(new) => { + n += 1; + new.clone() + } + None => part.trim().to_string(), + } + }) + .collect(); + if n == 0 { + return (line.to_string(), 0); + } + ( + format!( + "{}[{}]{}", + &line[..at + key.len() + open], + renamed.join(", "), + &rest[open + close + 1..] + ), + n, + ) +} + +/// Renames the keys of a `credit_overrides: { A: 0.5 }` mapping. +fn rename_braced(line: &str, key: &str, item: &BTreeMap) -> (String, usize) { + let Some(at) = key_at_word(line, key) else { + return (line.to_string(), 0); + }; + let rest = &line[at + key.len()..]; + let Some(open) = rest.find('{') else { + return (line.to_string(), 0); + }; + let Some(close) = rest[open..].find('}') else { + return (line.to_string(), 0); + }; + let inside = &rest[open + 1..open + close]; + let mut n = 0; + let renamed: Vec = inside + .split(',') + .map(|pair| match pair.split_once(':') { + Some((name, value)) => { + let trimmed = name.trim().trim_matches(['\'', '"']); + match item.get(trimmed) { + Some(new) => { + n += 1; + format!("{new}: {}", value.trim()) + } + None => pair.trim().to_string(), + } + } + None => pair.trim().to_string(), + }) + .collect(); + if n == 0 { + return (line.to_string(), 0); + } + ( + format!( + "{}{{ {} }}{}", + &line[..at + key.len() + open], + renamed.join(", "), + &rest[open + close + 1..] + ), + n, + ) +} + +/// Where a key appears as a whole key rather than as the tail of a longer one. +fn key_at_word(line: &str, key: &str) -> Option { + let at = line.find(key)?; + let before = &line[..at]; + if before + .chars() + .next_back() + .is_some_and(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return None; + } + Some(at) +} + +/// Drops the fields that a version number used to carry. +/// +/// `version:` on an item, `version:` on a placement, and the whole `history:` +/// block. All three still load and are ignored, so this is tidying rather than +/// repair — but a field that is read and thrown away is a field the next person +/// will keep maintaining. +/// +/// Textual, and deliberately narrow: only a `version:` key at the depth an item +/// or a placement declares one, and only a `history:` block and the lines +/// indented under it. A `version` inside a stem, a note, or an option's text is +/// not a key and is left alone. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `write` - whether to write, or only to count. +/// +/// # Returns +/// +/// One entry per file that changes, with how many lines it drops. +/// +/// # Errors +/// +/// Propagates read and write failures. +pub fn stems(root: &Path, write: bool) -> Result> { + let layout = Layout::new(root); + let mut out = Vec::new(); + + for dir in [layout.banks(), layout.assessments(), layout.seals()] { + for path in yaml::list_yaml(&dir)? { + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let rewritten = drop_versioning(&text); + if rewritten == text { + continue; + } + let dropped = text.lines().count() - rewritten.lines().count(); + if write { + yaml::write_text(&path, &rewritten)?; + } + out.push((relative(root, &path), dropped)); + } + } + Ok(out) +} + +/// Removes `version:` keys and `history:` blocks from a YAML document. +fn drop_versioning(text: &str) -> String { + let mut out: Vec<&str> = Vec::new(); + let mut skipping: Option = None; + + for line in text.lines() { + if let Some(depth) = skipping { + match indent_of(line) { + Some(n) if n > depth => continue, + None => continue, + _ => skipping = None, + } + } + let indent = match indent_of(line) { + Some(n) => n, + None => { + out.push(line); + continue; + } + }; + // `- version: 2` is a list item, which `history:` entries are; the key + // form is what an item or a placement writes. + let trimmed = line.trim_start().trim_start_matches("- "); + if trimmed == "history:" || trimmed.starts_with("history:") { + skipping = Some(indent); + continue; + } + if is_key(trimmed, "version") { + continue; + } + out.push(line); + } + + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + joined +} + +/// Whether a trimmed line declares exactly this key. +fn is_key(trimmed: &str, key: &str) -> bool { + match trimmed.strip_prefix(key) { + Some(rest) => rest.starts_with(':'), + None => false, + } +} + +/// One block of YAML: a key, the lines under it, and the comments above it. +#[derive(Debug, Clone, Default)] +struct Block { + /// The key, unquoted. + key: String, + /// The key line and everything indented under it, verbatim. + lines: Vec, + /// Comment lines that sat directly above the key. + lead: Vec, +} + +impl Block { + /// The block as text, comments first. + fn text(&self) -> String { + join(&self.lead, &self.lines) + } + + /// The lines under the key, without the key line. + fn body(&self) -> Vec { + self.lines.iter().skip(1).cloned().collect() + } +} + +/// Splits lines into the blocks keyed at one indentation. +fn blocks(lines: &[String], indent: usize) -> Vec { + let mut out: Vec = Vec::new(); + let mut current: Option = None; + let mut lead: Vec = Vec::new(); + + for line in lines { + if let Some(key) = key_at(line, indent) { + if let Some(mut block) = current.take() { + lead = detach_trailing_comments(&mut block); + out.push(block); + } + current = Some(Block { + key, + lines: vec![line.clone()], + lead: std::mem::take(&mut lead), + }); + continue; + } + match current.as_mut() { + Some(block) => block.lines.push(line.clone()), + // Comments above the first key belong to it. + None if line.trim().starts_with('#') => lead.push(line.clone()), + None => {} + } + } + if let Some(block) = current { + out.push(block); + } + out +} + +/// Moves a block's trailing comment run out, for the block that follows it. +/// +/// Trailing blank lines are dropped rather than carried: the caller writes its +/// own separators. +fn detach_trailing_comments(block: &mut Block) -> Vec { + let mut tail: Vec = Vec::new(); + while let Some(last) = block.lines.last() { + let trimmed = last.trim(); + if trimmed.is_empty() || trimmed.starts_with('#') { + tail.push(block.lines.pop().unwrap_or_default()); + } else { + break; + } + } + tail.reverse(); + match tail.iter().position(|l| l.trim().starts_with('#')) { + Some(first) => tail[first..].to_vec(), + None => Vec::new(), + } +} + +/// The key a line declares at an exact indentation, if it declares one. +fn key_at(line: &str, indent: usize) -> Option { + let prefix = line.get(..indent)?; + if !prefix.chars().all(|c| c == ' ') { + return None; + } + let rest = &line[indent..]; + if rest.is_empty() || rest.starts_with(' ') || rest.starts_with('#') || rest.starts_with('-') { + return None; + } + let colon = rest.find(':')?; + let after = &rest[colon + 1..]; + if !(after.is_empty() || after.starts_with(' ')) { + return None; + } + let key = rest[..colon].trim(); + if key.is_empty() { + return None; + } + Some(key.trim_matches(['\'', '"']).to_string()) +} + +/// The leading spaces of a line, or `None` for a blank one. +fn indent_of(line: &str) -> Option { + if line.trim().is_empty() { + return None; + } + Some(line.len() - line.trim_start().len()) +} + +/// Drops a key and everything indented under it. +fn without_key(lines: &[String], indent: usize, key: &str) -> Vec { + let mut out = Vec::new(); + let mut skipping = false; + for line in lines { + if skipping { + match indent_of(line) { + Some(n) if n > indent => continue, + _ => skipping = false, + } + } + if key_at(line, indent).as_deref() == Some(key) { + skipping = true; + continue; + } + out.push(line.clone()); + } + out +} + +/// Inserts a `teaches:` list into one lecture block. +/// +/// It goes above `readings:` when there is one, since the reading list is the +/// long part and a reader should not have to scroll past it to see what the +/// lecture covers. +fn with_teaches(lines: &[String], objectives: &[&str]) -> Vec { + let flow = format!(" teaches: [{}]", objectives.join(", ")); + let rendered: Vec = if flow.len() <= FLOW_WIDTH { + vec![flow] + } else { + let mut out = vec![" teaches:".to_string()]; + out.extend(objectives.iter().map(|o| format!(" - {o}"))); + out + }; + + let at = lines + .iter() + .position(|l| key_at(l, 4).as_deref() == Some("readings")) + .unwrap_or(lines.len()); + + let mut out: Vec = lines[..at].to_vec(); + while out.last().map(|l| l.trim().is_empty()).unwrap_or(false) { + out.pop(); + } + out.extend(rendered); + out.extend_from_slice(&lines[at..]); + out +} + +/// Drops a `# === label` group header, which the file it is moving into is now +/// entirely about. +fn strip_group_header(lead: &[String], labels: &[&str]) -> Vec { + lead.iter() + .filter(|line| { + let bare = line + .trim() + .trim_start_matches('#') + .trim_matches(|c: char| c == '=' || c.is_whitespace()); + !labels.contains(&bare) + }) + .cloned() + .collect() +} + +/// Joins a lead-comment run and a body into text. +fn join(lead: &[String], body: &[String]) -> String { + let mut out = String::new(); + for line in lead.iter().chain(body.iter()) { + out.push_str(line); + out.push('\n'); + } + out +} + +/// Collapses runs of blank lines and ensures exactly one trailing newline. +fn tidy(text: &str) -> String { + let mut out: Vec<&str> = Vec::new(); + for line in text.lines() { + if line.trim().is_empty() && out.last().map(|l| l.trim().is_empty()).unwrap_or(true) { + continue; + } + out.push(line); + } + while out.last().map(|l| l.trim().is_empty()).unwrap_or(false) { + out.pop(); + } + format!("{}\n", out.join("\n")) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::course::fragment; + + const MONOLITH: &str = r#"schema_version: '1.0' + +course: + code: BIOSC 1540 + title: Computational Biology + term: 2026f + +policy: + points_per_item: 1.0 + +units: + - id: u1 + title: Search and Similarity + +references: + ismail2023: + kind: book + title: Bioinformatics + authors: ['Ismail, H. D.'] + +lectures: + L1.2: + title: 'The Digital Genome' + unit: u1 + readings: + + - ref: ismail2023 + locator: 'ch. 1, §1.4' + targets: [t-fastq-structure] + summary: >- + The four-line FASTQ record, and how a quality score is packed into one + character. + +learning_objectives: + + # === L1.2 + lo-read-file-formats: + text: 'Read the text formats that carry sequences.' + unit: u1 + lectures: [L1.2] + order: 2 + level_ceiling: 2 + assessed: true + +learning_targets: + + # === lo-read-file-formats + t-fastq-structure: + text: 'Identify the four lines of a FASTQ record.' + objective: lo-read-file-formats + lectures: [L1.2] + order: 2 + assessed: true +"#; + + fn tmp(tag: &str) -> PathBuf { + let p = std::env::temp_dir().join(format!("coursebank-split-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&p); + std::fs::create_dir_all(&p).unwrap(); + p + } + + fn course_dir(tag: &str) -> PathBuf { + let root = tmp(tag); + std::fs::write(root.join(COURSE_FILE), MONOLITH).unwrap(); + root + } + + #[test] + fn a_split_reassembles_into_the_same_course() { + let root = course_dir("roundtrip"); + let before = CourseFile::load(&root.join(COURSE_FILE)).unwrap(); + + let plan = split_plan(&root).unwrap(); + apply(&root, &plan).unwrap(); + + let after = fragment::assemble(&root).unwrap(); + let diffs = differences(&before, &after).unwrap(); + assert!(diffs.is_empty(), "{diffs:?}"); + assert!(after.validate().is_empty(), "{:?}", after.validate()); + } + + #[test] + fn the_split_writes_one_file_per_lecture_and_objective() { + let root = course_dir("files"); + let plan = split_plan(&root).unwrap(); + apply(&root, &plan).unwrap(); + + assert!(root.join("references.yaml").is_file()); + assert!(root.join("lectures/l-1-2.yaml").is_file()); + assert!(root.join("objectives/lo-read-file-formats.yaml").is_file()); + assert!(root.join("course.yaml.bak").is_file()); + + let root_text = std::fs::read_to_string(root.join(COURSE_FILE)).unwrap(); + assert!(root_text.contains("course:")); + assert!(root_text.contains("units:")); + assert!(!root_text.contains("learning_objectives:")); + assert!(!root_text.contains("references:")); + } + + #[test] + fn prose_and_comments_survive_verbatim() { + let root = course_dir("prose"); + let plan = split_plan(&root).unwrap(); + apply(&root, &plan).unwrap(); + + let lecture = std::fs::read_to_string(root.join("lectures/l-1-2.yaml")).unwrap(); + // The folded scalar is still folded, and still wrapped where it was. + assert!(lecture.contains("summary: >-")); + assert!( + lecture + .contains("The four-line FASTQ record, and how a quality score is packed into one") + ); + assert!(lecture.contains("teaches: [lo-read-file-formats]")); + + // A `# === L1.2` header is about the file it now lives in, so it goes. + let objective = + std::fs::read_to_string(root.join("objectives/lo-read-file-formats.yaml")).unwrap(); + assert!(!objective.contains("# ==="), "{objective}"); + } + + #[test] + fn derived_lecture_lists_are_dropped_from_the_fragments() { + let root = course_dir("derived"); + let plan = split_plan(&root).unwrap(); + apply(&root, &plan).unwrap(); + + let objective = + std::fs::read_to_string(root.join("objectives/lo-read-file-formats.yaml")).unwrap(); + assert!(!objective.contains("lectures:"), "{objective}"); + assert!(objective.contains("order: 2")); + assert!(objective.contains("objective: lo-read-file-formats")); + } + + #[test] + fn splitting_twice_is_refused() { + let root = course_dir("twice"); + let plan = split_plan(&root).unwrap(); + apply(&root, &plan).unwrap(); + let message = split_plan(&root).unwrap_err().to_string(); + assert!(message.contains("already"), "{message}"); + } + + /// The same course, with the objective taught twice and the target reached + /// in only one of the two sessions. + fn two_lecture_monolith() -> String { + MONOLITH + .replace( + " lectures: [L1.2]\n order: 2\n level_ceiling: 2", + " lectures: [L1.2, L1.3]\n order: 2\n level_ceiling: 2", + ) + .replace( + "learning_objectives:", + " L1.3:\n title: 'Sequence similarity'\n\nlearning_objectives:", + ) + } + + #[test] + fn a_narrower_target_keeps_its_own_lectures() { + let root = tmp("narrower"); + std::fs::write(root.join(COURSE_FILE), two_lecture_monolith()).unwrap(); + + let plan = split_plan(&root).unwrap(); + let objective = plan + .files + .iter() + .find(|f| f.path.ends_with("lo-read-file-formats.yaml")) + .unwrap(); + // One `lectures:` left in the file: the objective's was derived away, + // the target's says something the objective does not. + assert_eq!( + objective.text.matches("lectures:").count(), + 1, + "{}", + objective.text + ); + assert!( + objective.text.contains("lectures: [L1.2]"), + "{}", + objective.text + ); + } + + #[test] + fn an_objective_taught_twice_is_registered_by_both_lectures() { + let root = tmp("twice-taught"); + std::fs::write(root.join(COURSE_FILE), two_lecture_monolith()).unwrap(); + let before = CourseFile::load(&root.join(COURSE_FILE)).unwrap(); + + let plan = split_plan(&root).unwrap(); + apply(&root, &plan).unwrap(); + + for file in ["lectures/l-1-2.yaml", "lectures/l-1-3.yaml"] { + let text = std::fs::read_to_string(root.join(file)).unwrap(); + assert!( + text.contains("teaches: [lo-read-file-formats]"), + "{file}: {text}" + ); + } + + let after = fragment::assemble(&root).unwrap(); + let diffs = differences(&before, &after).unwrap(); + assert!(diffs.is_empty(), "{diffs:?}"); + } + + #[test] + fn a_long_teaches_list_becomes_a_block_list() { + let many: Vec = (0..8) + .map(|i| format!("lo-a-fairly-long-objective-{i}")) + .collect(); + let refs: Vec<&str> = many.iter().map(String::as_str).collect(); + let out = with_teaches( + &[" L1.2:".to_string(), " title: One".to_string()], + &refs, + ); + assert_eq!(out[2], " teaches:"); + assert_eq!(out[3], " - lo-a-fairly-long-objective-0"); + } + + #[test] + fn the_scanner_finds_keys_only_at_its_own_depth() { + assert_eq!(key_at("lectures:", 0).as_deref(), Some("lectures")); + assert_eq!(key_at(" L1.2:", 2).as_deref(), Some("L1.2")); + assert_eq!(key_at(" 'A+': 1", 2).as_deref(), Some("A+")); + assert_eq!(key_at(" L1.2:", 0), None); + assert_eq!(key_at(" title: x", 2), None); + assert_eq!(key_at(" # comment", 2), None); + assert_eq!(key_at(" - id: u1", 2), None); + // A URL in a value is not a key. + assert_eq!( + key_at(" slides_url: https://x.test/a", 2).as_deref(), + Some("slides_url") + ); + } + + #[test] + fn a_qualified_item_id_is_stripped_in_both_yaml_styles() { + assert_eq!( + unqualify_line(" item: b-1-2::q-fastq-quality-line-001"), + " item: q-fastq-quality-line-001" + ); + assert_eq!( + unqualify_line(" - { number: 1, item: \"b1::q-a-001\", points: 1.5, key: [B] }"), + " - { number: 1, item: \"q-a-001\", points: 1.5, key: [B] }" + ); + assert_eq!(unqualify_line(" item: 'b::q-1'"), " item: 'q-1'"); + } + + #[test] + fn nothing_else_on_the_line_is_touched() { + // Already canonical. + assert_eq!(unqualify_line(" item: q-x"), " item: q-x"); + // A key that merely ends in `item`. + assert_eq!( + unqualify_line(" parent_item: b::q-1"), + " parent_item: b::q-1" + ); + // A `::` somewhere that is not an item id. + assert_eq!( + unqualify_line(" notes: 'see b::q-1 for the earlier wording'"), + " notes: 'see b::q-1 for the earlier wording'" + ); + // A digest that contains no qualifier. + assert_eq!( + unqualify_line(" fingerprint: 3f9a1cb2"), + " fingerprint: 3f9a1cb2" + ); + } + + #[test] + fn rewriting_ids_leaves_the_rest_of_the_record_alone() { + let root = tmp("ids"); + std::fs::create_dir_all(root.join("assessments")).unwrap(); + let record = r#"# assessments/a-1-2.yaml +schema_version: '1.0' +assessment: + id: A1.2 + title: 'A1.2 - The Digital Genome' +items: + - number: 1 + item: b-1-2::q-fastq-quality-line-001 + key: [D] +"#; + std::fs::write(root.join("assessments/a-1-2.yaml"), record).unwrap(); + + let changes = qualified_ids(&root).unwrap(); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].1, 1, "one line changes"); + apply_ids(&root, &changes).unwrap(); + + let after = std::fs::read_to_string(root.join("assessments/a-1-2.yaml")).unwrap(); + assert!(after.contains("item: q-fastq-quality-line-001"), "{after}"); + assert!(after.starts_with("# assessments/a-1-2.yaml"), "{after}"); + assert!( + after.contains("title: 'A1.2 - The Digital Genome'"), + "{after}" + ); + // Idempotent: a second pass finds nothing. + assert!(qualified_ids(&root).unwrap().is_empty()); + } + + #[test] + fn option_ids_come_from_the_leading_words() { + let ids = option_ids(&[ + "The first line".to_string(), + "The second line".to_string(), + "The third line".to_string(), + "The fourth line".to_string(), + ]); + assert_eq!( + ids, + vec![ + "o-first-line", + "o-second-line", + "o-third-line", + "o-fourth-line" + ] + ); + } + + #[test] + fn colliding_options_are_extended_by_what_tells_them_apart() { + let ids = option_ids(&[ + "Its error probability falls to about one half of the previous value".to_string(), + "Its error probability rises to about ten times the previous value".to_string(), + "Its error probability falls to about one tenth of the previous value".to_string(), + "Its error probability falls to about one hundredth of the previous value".to_string(), + ]); + // Every member of the colliding group is extended, including the last: + // a bare prefix beside two extended siblings reads like an oversight. + assert!(ids[0].ends_with("-half"), "{ids:?}"); + assert!(ids[2].ends_with("-tenth"), "{ids:?}"); + assert!(ids[3].ends_with("-hundredth"), "{ids:?}"); + assert_eq!(ids.iter().collect::>().len(), 4); + } + + #[test] + fn math_survives_long_enough_to_tell_two_options_apart() { + let ids = option_ids(&[ + "Alignment 2; score $-1$".to_string(), + "Alignment 1; score $3$".to_string(), + "Alignment 1; score $2$".to_string(), + "Alignment 2; score $5$".to_string(), + ]); + assert_eq!(ids.iter().collect::>().len(), 4, "{ids:?}"); + assert!(ids[0].contains("minus"), "{ids:?}"); + } + + #[test] + fn options_nothing_tells_apart_get_no_id_at_all() { + let ids = option_ids(&["Yes".to_string(), "Yes".to_string()]); + assert!(ids.is_empty()); + } + + #[test] + fn an_id_does_not_end_on_a_function_word() { + let ids = option_ids(&["A descriptive identifier for the sequence".to_string()]); + assert_eq!(ids, vec!["o-descriptive-identifier"]); + } + + #[test] + fn renaming_a_bank_touches_only_option_ids() { + let bank = r#"items: + - id: q-x + stem: Which line? + options: + - id: A + text: 'The first line' + - id: D + text: 'The fourth line' +"#; + let mut map: OptionMap = BTreeMap::new(); + map.insert( + "q-x".to_string(), + [ + ("A".to_string(), "o-first-line".to_string()), + ("D".to_string(), "o-fourth-line".to_string()), + ] + .into_iter() + .collect(), + ); + + let (out, n) = rename_in_bank(bank, &map); + assert_eq!(n, 2); + assert!(out.contains(" - id: o-first-line"), "{out}"); + assert!(out.contains(" - id: o-fourth-line"), "{out}"); + // The item id, the stem, and the option text are untouched. + assert!(out.contains(" - id: q-x"), "{out}"); + assert!(out.contains("text: 'The fourth line'"), "{out}"); + } + + #[test] + fn renaming_a_record_rewrites_the_key_and_the_overrides() { + let mut map: OptionMap = BTreeMap::new(); + map.insert( + "q-x".to_string(), + [ + ("B".to_string(), "o-second-line".to_string()), + ("D".to_string(), "o-fourth-line".to_string()), + ] + .into_iter() + .collect(), + ); + + let block = r#"items: + - number: 1 + item: q-x + key: [D] + credit_overrides: { B: 0.5 } + level: 1 +"#; + let (out, n) = rename_in_record(block, &map); + assert_eq!(n, 2, "{out}"); + assert!(out.contains("key: [o-fourth-line]"), "{out}"); + assert!( + out.contains("credit_overrides: { o-second-line: 0.5 }"), + "{out}" + ); + assert!(out.contains("level: 1"), "{out}"); + + // The flow form the older records use, with a pre-2.0 qualified id. + let flow = " - { number: 1, item: \"b1::q-x\", key: [B], level: 1 }\n"; + let (out, n) = rename_in_record(flow, &map); + assert_eq!(n, 1, "{out}"); + assert!(out.contains("key: [o-second-line]"), "{out}"); + assert!(out.contains("item: \"b1::q-x\""), "{out}"); + } + + #[test] + fn a_stored_response_column_is_renamed_member_by_member() { + let item: BTreeMap = [ + ("A".to_string(), "o-first".to_string()), + ("C".to_string(), "o-third".to_string()), + ] + .into_iter() + .collect(); + assert_eq!(rename_list("A", &item), "o-first"); + assert_eq!(rename_list("A;C", &item), "o-first;o-third"); + // An option with no mapping is left as it is rather than dropped. + assert_eq!(rename_list("A;Z", &item), "o-first;Z"); + assert_eq!(rename_list("", &item), ""); + } + + #[test] + fn versioning_is_dropped_key_by_key() { + let bank = r#"items: + - id: q-x + version: 1 + status: approved + stem: >- + Which version of the file format is this? + options: + - id: o-a + text: 'The version line' + history: + - version: 2 + date: 2026-01-01 + change: reworded + - version: 1 + date: 2025-12-01 + change: written + topics: [formats] +"#; + let out = drop_versioning(bank); + assert!(!out.contains("version: 1"), "{out}"); + assert!(!out.contains("history:"), "{out}"); + assert!(!out.contains("change: reworded"), "{out}"); + // The stem and the option text mention a version and must survive. + assert!( + out.contains("Which version of the file format is this?"), + "{out}" + ); + assert!(out.contains("text: 'The version line'"), "{out}"); + assert!(out.contains("topics: [formats]"), "{out}"); + assert!(out.contains("status: approved"), "{out}"); + } + + #[test] + fn a_placement_version_goes_too() { + let record = "items:\n - number: 1\n item: q-x\n version: 1\n key: [o-a]\n"; + let out = drop_versioning(record); + assert_eq!( + out, + "items:\n - number: 1\n item: q-x\n key: [o-a]\n" + ); + } + + #[test] + fn lecture_slugs_match_the_house_naming() { + assert_eq!(lecture_slug("L1.2"), "l-1-2"); + assert_eq!(lecture_slug("L11"), "l-11"); + assert_eq!(lecture_slug("Week 3"), "week-3"); + } +} diff --git a/src/model.rs b/src/model.rs index 68a0a71..ae66e04 100644 --- a/src/model.rs +++ b/src/model.rs @@ -12,7 +12,9 @@ //! ```text //! taxonomy levels, cognitive processes, error types, status, flags //! │ -//! course course.yaml: identity, policy, objectives, lectures, stimuli +//! course identity, policy, objectives, lectures, stimuli +//! │ └─ course::fragment merges course.yaml, references.yaml, +//! │ lectures/*.yaml, objectives/*.yaml //! │ //! item one question: stem, options, design intent, calibration //! │ diff --git a/src/model/assessment.rs b/src/model/assessment.rs index b217535..cf46986 100644 --- a/src/model/assessment.rs +++ b/src/model/assessment.rs @@ -248,11 +248,28 @@ pub struct Placement { /// Printed question number. This is the join key to grading exports, which /// is the entire reason this record exists. pub number: u32, - /// The item's global id, `bank::item`. + /// The item's id, which names it course-wide. A pre-2.0 `bank::item` + /// value still resolves; `coursebank migrate ids` rewrites it. pub item: String, /// The item version used. - #[serde(default, skip_serializing_if = "Option::is_none")] + /// Retained only so a pre-2.0 record still loads. Ignored. + /// + /// What it was for — knowing whether the item has changed since this + /// administration — is [`Placement::stem_digest`] and + /// [`Placement::fingerprint`], which say *what* changed rather than that + /// something did. + #[serde(default, skip_serializing)] pub version: Option, + + /// The stem's digest as administered. See [`crate::item::Item::stem_digest`]. + /// + /// Distinct from `fingerprint`, which covers the options too. A changed + /// fingerprint means the pooled statistics describe an older wording; a + /// changed stem digest means this is no longer the same question, which + /// [`crate::catalog::Catalog::validate_record`] treats as an error rather + /// than a note. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub stem_digest: Option, /// The content fingerprint as used, so later edits are detectable. #[serde(default, skip_serializing_if = "Option::is_none")] pub fingerprint: Option, diff --git a/src/model/bank.rs b/src/model/bank.rs index 90e3cac..6694305 100644 --- a/src/model/bank.rs +++ b/src/model/bank.rs @@ -352,8 +352,10 @@ fn validate_item( if it.stem.trim().is_empty() { issues.push("empty stem".into()); } - if it.version == 0 { - issues.push("version must be at least 1".into()); + if let Some(replaced) = &it.supersedes { + if *replaced == it.id { + issues.push("supersedes names this item".into()); + } } // --- options ---- @@ -379,15 +381,22 @@ fn validate_item( if o.text.trim().is_empty() { issues.push(format!("option {pos}: empty text")); } - let letter_ok = o.id.len() == 1 - && o.id - .chars() - .next() - .map(|c| c.is_ascii_uppercase() && c <= 'H') - .unwrap_or(false); - if !letter_ok { + // Two forms are accepted: the 2.0 name, and the letter that preceded + // it. A bank migrates when `coursebank migrate options` is run on it, + // not when the tool is upgraded, so a course mid-migration still loads. + let slug_ok = o.id.strip_prefix("o-").is_some_and(|rest| { + !rest.is_empty() + && !rest.starts_with('-') + && !rest.ends_with('-') + && !rest.contains("--") + && rest + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-') + }); + if !slug_ok && !Item::is_legacy_option_id(&o.id) { issues.push(format!( - "option {pos}: id `{}` must be a single letter A through H", + "option {pos}: id `{}` is neither a name such as `o-fourth-line` nor a pre-2.0 \ + letter A through H", o.id )); } @@ -514,10 +523,10 @@ fn validate_item( )); } } - for letter in c.option_stats.keys() { - if it.option(letter).is_none() { + for option in c.option_stats.keys() { + if it.option(option).is_none() { issues.push(format!( - "calibration.option_stats has `{letter}`, which is not an option of this item" + "calibration.option_stats has `{option}`, which is not an option of this item" )); } } @@ -533,25 +542,6 @@ fn validate_item( } } - // --- history must be coherent ------ - let mut last_version = 0u32; - for (i, h) in it.history.iter().enumerate() { - if h.version <= last_version { - issues.push(format!( - "history entry {} has version {} which does not increase", - i + 1, - h.version - )); - } - last_version = h.version; - } - if !it.history.is_empty() && last_version > it.version { - issues.push(format!( - "history records version {last_version} but the item says version {}", - it.version - )); - } - // --- retirement ----- if it.retired.is_some() && it.status != Status::Retired { issues.push(format!( @@ -1027,27 +1017,6 @@ learning_targets: ); } - #[test] - fn history_versions_must_increase() { - let b = bank( - r#" - - id: q-a-001 - version: 2 - status: draft - level: 1 - stem: s - options: - - { id: A, text: a, correct: true } - - { id: B, text: b } - history: - - { version: 2, date: 2026-01-01, change: second } - - { version: 1, date: 2026-01-02, change: first } -"#, - ); - let issues = b.validate(None); - assert!(issues.iter().any(|i| i.contains("does not increase"))); - } - #[test] fn level_counts_exclude_drafts_and_bonuses() { let b = bank( diff --git a/src/model/catalog.rs b/src/model/catalog.rs index 47f34ff..3abe0c3 100644 --- a/src/model/catalog.rs +++ b/src/model/catalog.rs @@ -5,7 +5,7 @@ //! Loading a whole course at once, and reporting on what it contains. //! //! A [`Catalog`] is every bank in a course, indexed so that an item can be found -//! by its global id (`bank::item`), and so that questions like "how many Apply +//! by its id, and so that questions like "how many Apply //! level items do I have on lecture 12" have a cheap answer. //! //! The global id is the join key for everything downstream: assessment records @@ -32,7 +32,12 @@ use crate::yaml; /// One item plus everything needed to locate it again. #[derive(Debug, Clone)] pub struct Entry { - /// The globally unique id, `bank::item`. + /// The item's id, which names it course-wide. + /// + /// The bank is in [`Entry::bank`] and the file in [`Entry::path`], neither + /// of which is part of the identity: this is the join key that response + /// data carries, and a join key with a file name in it renames itself every + /// time the files are reorganized. pub uid: String, /// The bank id. pub bank: String, @@ -92,7 +97,7 @@ impl Catalog { /// id, since either makes the join key ambiguous. pub fn load(root: &Path) -> Result { let layout = Layout::new(root); - let course = CourseFile::load(&layout.course_file())?; + let course = CourseFile::load_dir(root)?; let mut catalog = Catalog { course, layout, @@ -117,9 +122,13 @@ impl Catalog { } catalog.banks.insert(bank_id.clone(), bank.bank.clone()); for (i, item) in bank.items.into_iter().enumerate() { - let uid = format!("{bank_id}::{}", item.id); - if catalog.index.contains_key(&uid) { - problems.push(format!("duplicate global item id `{uid}`")); + let uid = item.id.clone(); + if let Some(first) = catalog.index.get(&uid) { + problems.push(format!( + "item id `{uid}` is used twice: in bank `{}` and in bank `{bank_id}`. An \ + id names one question course-wide, because response data joins on it.", + catalog.entries[*first].bank + )); continue; } catalog.index.insert(uid.clone(), catalog.entries.len()); @@ -141,15 +150,22 @@ impl Catalog { /// Looks up an item by global id. /// + /// A pre-2.0 `bank::item` id resolves to the item it used to name, so an + /// assessment record or a parquet file written before the change still + /// joins. See [`crate::item::canonical_id`]. + /// /// # Arguments /// - /// * `uid` - the global id, `bank::item`. + /// * `uid` - the item id, in either form. /// /// # Returns /// /// The entry, or `None`. pub fn get(&self, uid: &str) -> Option<&Entry> { - self.index.get(uid).map(|i| &self.entries[*i]) + self.index + .get(uid) + .or_else(|| self.index.get(crate::item::canonical_id(uid))) + .map(|i| &self.entries[*i]) } /// Looks up an item by global id, erroring when absent. @@ -173,45 +189,37 @@ impl Catalog { }) } - /// Resolves a possibly-unqualified id to a global id. + /// Resolves an id written in either form to the canonical one. /// - /// Typing `q-mm-kinetics-001` on the command line should work when that id is - /// unambiguous across the course, because remembering which bank a question - /// lives in is exactly the sort of bookkeeping this tool exists to remove. + /// Since 2.0 an item id is already course-wide, so this is the identity for + /// anything current. What it is still for is the old `bank::item` form, + /// which appears in assessment records, seals, and response files written + /// before the change, and which a person may well still type. /// /// # Arguments /// - /// * `id` - a global id, or a bare item id. + /// * `id` - an item id, in either form. /// /// # Returns /// - /// The global id. + /// The canonical id. /// /// # Errors /// - /// Returns [`Error::Unresolved`] when nothing matches, or [`Error::Usage`] - /// when a bare id matches items in more than one bank. + /// Returns [`Error::Unresolved`] when nothing matches. pub fn resolve(&self, id: &str) -> Result { if self.index.contains_key(id) { return Ok(id.to_string()); } - let matches: Vec<&Entry> = self.entries.iter().filter(|e| e.item.id == id).collect(); - match matches.len() { - 0 => Err(Error::Unresolved { - kind: "item", - id: id.to_string(), - context: None, - }), - 1 => Ok(matches[0].uid.clone()), - _ => Err(Error::usage(format!( - "`{id}` is ambiguous; it exists in {}. Use the full `bank::item` form.", - matches - .iter() - .map(|e| e.bank.as_str()) - .collect::>() - .join(", ") - ))), + let canonical = crate::item::canonical_id(id); + if self.index.contains_key(canonical) { + return Ok(canonical.to_string()); } + Err(Error::Unresolved { + kind: "item", + id: id.to_string(), + context: None, + }) } /// Every item that may be placed on a graded assessment. @@ -232,12 +240,10 @@ impl Catalog { /// /// Problems, prefixed with the file they came from. pub fn validate(&self) -> Result> { - let mut issues: Vec = self - .course - .validate() - .into_iter() - .map(|m| format!("course.yaml: {m}")) - .collect(); + // Not prefixed here: `CourseFile::validate` attributes each message to + // the fragment that defined the id, which for an unsplit course is + // `course.yaml` and for a split one is the file worth opening. + let mut issues: Vec = self.course.validate(); for path in yaml::list_yaml(&self.layout.banks())? { let bank = BankFile::load_resolved(&path)?; @@ -255,6 +261,45 @@ impl Catalog { Ok(issues) } + /// Checks every sealed administration's stems against the bank. + /// + /// The seal is the authority on what was administered, so this is the + /// comparison that matters: a record can be edited, but a seal is written + /// before the exam is printed and digested against tampering. A stem that + /// no longer matches the one a cohort answered means the id now names a + /// different question, and every statistic pooled under it is describing + /// two things at once. + /// + /// # Arguments + /// + /// * `seals` - the sealed administrations to check. + /// + /// # Returns + /// + /// One message per stem that has moved out from under its seal. + pub fn validate_seals(&self, seals: &[crate::seal::SealFile]) -> Vec { + let mut issues = Vec::new(); + for seal in seals { + for item in &seal.items { + let Some(digest) = &item.stem_digest else { + continue; + }; + let Some(entry) = self.get(&item.item) else { + continue; + }; + if *digest != entry.item.stem_digest() { + issues.push(format!( + "{}: question {} (`{}`) was administered with a different stem than the \ + bank now holds. Statistics from that administration describe the older \ + wording; give the new wording its own id.", + seal.seal.assessment, item.number, item.item + )); + } + } + } + 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. @@ -264,6 +309,22 @@ impl Catalog { match self.get(&p.item) { None => issues.push(format!("question {}: unknown item `{}`", p.number, p.item)), Some(entry) => { + // The rule the stem digest exists to enforce. A changed + // fingerprint is a note: the statistics describe an older + // wording. A changed stem is an error: whatever was + // administered is not the question the bank now holds, so + // the id is being reused for two different questions. + if let Some(digest) = &p.stem_digest { + if *digest != entry.item.stem_digest() { + issues.push(format!( + "question {} ({}): the stem has been reworded since this \ + assessment. A reworded stem is a new question: give the new \ + wording a new id with `supersedes: {}`, and leave this one as \ + it was administered.", + p.number, p.item, p.item + )); + } + } if let Some(fp) = &p.fingerprint { if *fp != entry.item.fingerprint() { issues.push(format!( @@ -705,13 +766,32 @@ items: let cat = Catalog::load(&dir).expect("catalog loads"); assert_eq!(cat.entries.len(), 1); - assert_eq!(cat.entries[0].uid, "b1::q-x-001"); + // The id names the item course-wide; the bank is where it is kept. + assert_eq!(cat.entries[0].uid, "q-x-001"); + assert_eq!(cat.entries[0].bank, "b1"); + assert!(cat.get("q-x-001").is_some()); + assert_eq!(cat.resolve("q-x-001").unwrap(), "q-x-001"); + // A record or a parquet file written before 2.0 still joins. assert!(cat.get("b1::q-x-001").is_some()); - assert_eq!(cat.resolve("q-x-001").unwrap(), "b1::q-x-001"); + assert_eq!(cat.resolve("b1::q-x-001").unwrap(), "q-x-001"); + assert!(cat.get("b1::q-nonexistent").is_none()); assert!(cat.validate().unwrap().is_empty()); let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn one_item_id_in_two_banks_is_fatal() { + let dir = tmp("dupitem"); + write_course(&dir, ""); + write_bank(&dir, "b1.yaml", APPROVED); + write_bank(&dir, "b2.yaml", &APPROVED.replace("id: b1", "id: b2")); + + let message = Catalog::load(&dir).unwrap_err().to_string(); + assert!(message.contains("`q-x-001` is used twice"), "{message}"); + assert!(message.contains("course-wide"), "{message}"); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn duplicate_bank_ids_are_fatal() { let dir = tmp("dupbank"); @@ -723,21 +803,6 @@ items: let _ = std::fs::remove_dir_all(&dir); } - #[test] - fn ambiguous_bare_ids_are_rejected() { - let dir = tmp("ambig"); - write_course(&dir, ""); - write_bank(&dir, "a.yaml", APPROVED); - write_bank(&dir, "b.yaml", &APPROVED.replace("id: b1", "id: b2")); - let cat = Catalog::load(&dir).expect("distinct banks load"); - assert_eq!(cat.entries.len(), 2); - let err = cat.resolve("q-x-001").expect_err("bare id is ambiguous"); - assert!(format!("{err}").contains("ambiguous")); - // The fully qualified form still works. - assert_eq!(cat.resolve("b2::q-x-001").unwrap(), "b2::q-x-001"); - let _ = std::fs::remove_dir_all(&dir); - } - #[test] fn coverage_finds_real_gaps() { let dir = tmp("coverage"); diff --git a/src/model/course.rs b/src/model/course.rs index c9f1017..3d93428 100644 --- a/src/model/course.rs +++ b/src/model/course.rs @@ -4,20 +4,29 @@ //! The course file: identity plus the registries every bank references. //! -//! Learning objectives, their learning targets, and lectures are declared once, -//! in `course.yaml`, and referenced by id from items. That is the single most load-bearing decision in +//! Learning objectives, their learning targets, and lectures are declared once +//! and referenced by id from items. That is the single most load-bearing decision in //! the schema. It means an objective's wording lives in exactly one place, so //! rewording it updates every report; it means a report can name what a student //! missed by objective rather than by question number; and it means a dangling //! reference is a hard error instead of a silently misspelled string that splits //! your coverage table into two near-identical rows. //! +//! "Once" is a claim about ids, not about files. [`CourseFile`] is the resolved +//! model, and it may be assembled from a directory of fragments — one file per +//! lecture, one per objective — as well as from a single `course.yaml`. Either +//! way an id has exactly one definition site, and [`CourseFile::origins`] +//! records which file that was. See [`fragment`] for the merge and the rules +//! that keep it honest. +//! //! The course file also declares the term. Items live across terms, so the term //! belongs to the course and the administration, never to the item. +pub mod fragment; + use std::collections::BTreeMap; use std::fmt; -use std::path::Path; +use std::path::{Path, PathBuf}; use serde::de::{self, MapAccess, Visitor}; use serde::ser::SerializeMap; @@ -28,8 +37,19 @@ use crate::error::{Error, Result}; use crate::taxonomy::Level; use crate::yaml; +use fragment::Section; + /// The schema version this build of the tool writes. -pub const SCHEMA_VERSION: &str = "1.0"; +pub const SCHEMA_VERSION: &str = "2.0"; + +/// The schema major versions this build can read. +/// +/// A 1.0 repository loads unchanged. What 2.0 changes is the shape of two +/// things, and both are tolerated on the way in: a course file may be split +/// into fragments, and an item is named course-wide rather than as +/// `bank::item`. `coursebank migrate` rewrites files into the 2.0 form when you +/// are ready; nothing forces it. +pub const SUPPORTED_MAJORS: [&str; 2] = ["1", "2"]; /// The canonical file name inside a course directory. pub const COURSE_FILE: &str = "course.yaml"; @@ -91,6 +111,16 @@ pub struct CourseFile { /// Shared stimuli for case-based testlets, keyed by id. #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] pub stimuli: BTreeMap, + + /// Which file defined each id, relative to the course root. + /// + /// Populated by [`fragment::assemble`] and empty for a course parsed + /// straight out of one file by [`CourseFile::load`]. It is what makes a + /// validation message able to name the file to open, which matters rather a + /// lot once one course is forty files. Not serialized: it describes where + /// the model came from, not what it says. + #[serde(skip)] + pub origins: BTreeMap<(Section, String), PathBuf>, } /// Course identity. @@ -285,6 +315,15 @@ pub struct Lecture { /// Where the slides live, for study guidance in student reports. #[serde(default, skip_serializing_if = "Option::is_none")] pub slides_url: Option, + /// The objectives this session develops. + /// + /// The registration direction: you write what a lecture covers while + /// planning the lecture, and each named objective gains this lecture in its + /// [`Objective::lectures`] list during [`fragment::assemble`]. Declaring the + /// pair from the objective's side instead is equivalent, and declaring it + /// from both is redundant rather than contradictory — the two are unioned. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub teaches: Vec, /// Assigned readings for the session, in the order you assign them. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub readings: Vec, @@ -335,8 +374,21 @@ pub struct Reference { #[serde(default, skip_serializing_if = "Option::is_none")] pub pages: Option, /// DOI, bare: `10.1038/nature12373`. + /// + /// For a manuscript this is usually the only link worth storing: it is the + /// identifier of the work rather than of one copy of it, and [`Reference::href`] + /// turns it into a URL. #[serde(default, skip_serializing_if = "Option::is_none")] pub doi: Option, + /// arXiv id, bare: `2301.00001` or `q-bio/0501001`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub arxiv: Option, + /// PubMed Central id, which hosts the full text: `PMC3084216`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pmcid: Option, + /// PubMed id, which hosts a record about the work: `21471563`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pmid: Option, /// ISBN, for a book. #[serde(default, skip_serializing_if = "Option::is_none")] pub isbn: Option, @@ -353,6 +405,123 @@ pub struct Reference { pub note: Option, } +impl Reference { + /// Where to send a reader, most specific first. + /// + /// The one link resolution in the crate. Every exporter used to carry its + /// own copy of the `base_url` join, which meant a reading list, a printed + /// key, a practice sheet, and a student report could disagree about where a + /// citation points — and that none of them linked a journal article, since + /// an article has no `base_url` to join a path to. + /// + /// The order is from the exact location outward: a link to §1.4 beats a link + /// to the work, and a link to the work beats nothing. + /// + /// # Arguments + /// + /// * `url` - a full URL for the exact location, from a reading or citation. + /// * `path` - a location under this work's `base_url`. + /// + /// # Returns + /// + /// The most specific link available, or `None` for a work with no online + /// location at all. + pub fn href(&self, url: Option<&str>, path: Option<&str>) -> Option { + if let Some(url) = url { + return Some(url.to_string()); + } + if let (Some(base), Some(path)) = (self.base_url.as_deref(), path) { + return Some(join_url(base, path)); + } + if let Some(url) = &self.url { + return Some(url.clone()); + } + self.identifier_url() + } + + /// The link this work's identifiers resolve to, ignoring any location inside + /// it. + /// + /// DOI first, because it names the work rather than one copy of it. Then + /// arXiv and PubMed Central, which host the article itself, before PubMed, + /// which hosts a record about it. + /// + /// # Returns + /// + /// A URL, or `None` when the work carries no identifier. + pub fn identifier_url(&self) -> Option { + if let Some(doi) = self.doi.as_deref().map(bare_doi) { + return Some(format!("https://doi.org/{doi}")); + } + if let Some(id) = self.arxiv.as_deref().map(bare_arxiv) { + return Some(format!("https://arxiv.org/abs/{id}")); + } + if let Some(id) = self.pmcid.as_deref().map(str::trim) { + let id = if id.starts_with("PMC") { + id.to_string() + } else { + format!("PMC{id}") + }; + return Some(format!("https://www.ncbi.nlm.nih.gov/pmc/articles/{id}/")); + } + if let Some(id) = self.pmid.as_deref().map(str::trim) { + return Some(format!("https://pubmed.ncbi.nlm.nih.gov/{id}/")); + } + None + } + + /// The short form a reading list shows: the label, or the citation key. + /// + /// # Arguments + /// + /// * `key` - the citation key, used when the work declares no label. + /// + /// # Returns + /// + /// The label to print. + pub fn label_or<'a>(&'a self, key: &'a str) -> &'a str { + self.label.as_deref().unwrap_or(key) + } +} + +/// A DOI with any resolver prefix stripped, so `href` cannot produce +/// `https://doi.org/https://doi.org/10...`. +fn bare_doi(doi: &str) -> &str { + let doi = doi.trim(); + for prefix in [ + "https://doi.org/", + "http://doi.org/", + "https://dx.doi.org/", + "http://dx.doi.org/", + "doi:", + ] { + if let Some(rest) = doi.strip_prefix(prefix) { + return rest; + } + } + doi +} + +/// An arXiv id with the `arXiv:` prefix stripped. +fn bare_arxiv(id: &str) -> &str { + let id = id.trim(); + for prefix in ["arXiv:", "arxiv:", "https://arxiv.org/abs/"] { + if let Some(rest) = id.strip_prefix(prefix) { + return rest; + } + } + id +} + +/// Joins a base URL and a path without doubling or dropping the separator. +fn join_url(base: &str, path: &str) -> String { + match (base.ends_with('/'), path.starts_with('/')) { + (true, true) => format!("{base}{}", &path[1..]), + (false, false) => format!("{base}/{path}"), + _ => format!("{base}{path}"), + } +} + /// The kind of work, chosen to map onto BibTeX entry types. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] @@ -447,19 +616,10 @@ impl Reading { /// /// # Returns /// - /// `url` when given, otherwise the reference's `base_url` joined with `path`, - /// otherwise `None`. + /// The most specific link available, which for a manuscript with a DOI and + /// no `path` is the DOI. See [`Reference::href`]. pub fn resolve_url(&self, reference: &Reference) -> Option { - if let Some(url) = &self.url { - return Some(url.clone()); - } - let path = self.path.as_deref()?; - let base = reference.base_url.as_deref()?; - Some(match (base.ends_with('/'), path.starts_with('/')) { - (true, true) => format!("{base}{}", &path[1..]), - (false, false) => format!("{base}/{path}"), - _ => format!("{base}{path}"), - }) + reference.href(self.url.as_deref(), self.path.as_deref()) } /// A short citation for a report: `KKW §6.1`. @@ -476,7 +636,7 @@ impl Reading { if let Some(text) = &self.text { return text.clone(); } - let label = reference.label.as_deref().unwrap_or(key); + let label = reference.label_or(key); match &self.locator { Some(locator) => format!("{label} {locator}"), None => label.to_string(), @@ -761,7 +921,12 @@ impl CourseFile { yaml::read(path) } - /// Finds and loads the course file for a course directory. + /// Loads the course for a course directory, merging every fragment it holds. + /// + /// This is the entry point every command uses. A directory holding only + /// `course.yaml` gives the same result it always did; one that also holds + /// `lectures/`, `objectives/`, or `references.yaml` gets them merged in. See + /// [`fragment::assemble`]. /// /// # Arguments /// @@ -769,34 +934,167 @@ impl CourseFile { /// /// # Returns /// - /// The parsed course file. + /// The merged course file. /// /// # Errors /// - /// Propagates load errors, including absence of `course.yaml`. + /// Propagates load errors, including absence of `course.yaml`, and returns + /// [`Error::Invalid`] when two files define the same id. pub fn load_dir(dir: &Path) -> Result { - CourseFile::load(&dir.join(COURSE_FILE)) + fragment::assemble(dir) + } + + /// Which file defined an id, and which registry it was in. + /// + /// # Arguments + /// + /// * `id` - a unit, lecture, objective, target, reference, or stimulus id. + /// + /// # Returns + /// + /// The section and the path relative to the course root, or `None` for an + /// unknown id or a course that was not assembled from fragments. + pub fn origin(&self, id: &str) -> Option<(Section, &Path)> { + Section::ALL.iter().find_map(|section| { + self.origins + .get(&(*section, id.to_string())) + .map(|path| (*section, path.as_path())) + }) + } + + /// Every file this course was assembled from, in sorted order. + /// + /// # Returns + /// + /// The paths relative to the course root, empty for a course parsed from a + /// single file by [`CourseFile::load`]. + pub fn fragment_paths(&self) -> Vec<&Path> { + let mut paths: Vec<&Path> = self.origins.values().map(PathBuf::as_path).collect(); + paths.sort_unstable(); + paths.dedup(); + paths } /// Writes the course file back out as YAML. /// + /// Refuses to write a course that was assembled from more than one file, + /// because the merged model has no home on disk: writing it to + /// `course.yaml` would leave every fragment defining ids the root file also + /// defines, which is the one thing [`fragment::assemble`] treats as an + /// error. Use [`CourseFile::write_resolved`] for an inspection copy. + /// /// # Arguments /// /// * `path` - destination path. /// /// # Errors /// - /// Returns [`Error::Io`] on a write failure. + /// Returns [`Error::Usage`] for a fragmented course and [`Error::Io`] on a + /// write failure. pub fn save(&self, path: &Path) -> Result<()> { + let sources = self.fragment_paths(); + if sources.len() > 1 { + return Err(Error::usage(format!( + "this course is assembled from {} files, so it cannot be written back to one. \ + Edit the fragment that owns what you are changing, or use `coursebank course \ + build` for a merged copy.", + sources.len() + ))); + } yaml::write(path, self) } + /// Writes the merged course as YAML, for reading rather than for loading. + /// + /// The output carries a banner saying so. It is what `coursebank course + /// build` writes, and nothing in the tool reads it back: a generated file + /// that commands depend on is a file that goes stale. + /// + /// # Arguments + /// + /// * `path` - destination path. + /// + /// # Errors + /// + /// Returns [`Error::Io`] on a write failure, or [`Error::Other`] if the + /// model cannot be represented as YAML. + pub fn write_resolved(&self, path: &Path) -> Result<()> { + let body = yaml::to_string(self)?; + let banner = format!( + "# Generated by `coursebank course build` from {} file(s). Do not edit: nothing\n\ + # reads this, and the next build overwrites it. Edit the fragments instead.\n", + self.fragment_paths().len().max(1) + ); + yaml::write_text(path, &format!("{banner}{body}")) + } + /// Checks internal consistency of the registries. /// + /// Each message is prefixed with the file that defined the id it is about, + /// when that is known. For an unsplit course that is always `course.yaml`, + /// which is what the messages used to say. + /// /// # Returns /// /// Every problem found, empty when the file is sound. pub fn validate(&self) -> Vec { + self.problems() + .into_iter() + .map(|issue| self.attribute(issue)) + .collect() + } + + /// Prefixes one validation message with the fragment it concerns. + /// + /// The id is taken from the first backticked token in the message, since + /// every message that is about a registry entry names it first. Messages + /// about `course`, `policy`, or `units` are attributed by section instead, + /// because the first thing they quote is a field or a grade letter. + /// + /// # Arguments + /// + /// * `issue` - the message. + /// + /// # Returns + /// + /// The message, prefixed with a path when one is known. + fn attribute(&self, issue: String) -> String { + let section = if issue.starts_with("course.") { + Some(Section::Course) + } else if issue.starts_with("policy.") { + Some(Section::Policy) + } else if issue.starts_with("units") { + Some(Section::Units) + } else { + None + }; + + let path = match section { + Some(section) => self.section_origin(section), + None => issue + .split('`') + .nth(1) + .and_then(|id| self.origin(id)) + .map(|(_, path)| path), + }; + + match path { + Some(path) => format!("{}: {issue}", path.display()), + None => issue, + } + } + + /// The file that declared a whole section, for the sections that are not + /// keyed by id. + fn section_origin(&self, section: Section) -> Option<&Path> { + self.origins + .iter() + .find(|((s, _), _)| *s == section) + .map(|(_, path)| path.as_path()) + } + + /// The validation messages, before they are attributed to files. + fn problems(&self) -> Vec { let mut issues = Vec::new(); if self.course.code.trim().is_empty() { @@ -874,6 +1172,25 @@ impl CourseFile { if reference.title.trim().is_empty() { issues.push(format!("reference `{key}`: empty title")); } + // Checked rather than silently coerced: `href` strips a resolver + // prefix, but something that is not a DOI at all would become a + // link that 404s on a student's reading list. + if let Some(doi) = &reference.doi { + if !bare_doi(doi).starts_with("10.") { + issues.push(format!( + "reference `{key}`: `{doi}` is not a DOI. Write it bare, as \ + 10.1038/nature12373." + )); + } + } + if let Some(pmid) = &reference.pmid { + if !pmid.trim().chars().all(|c| c.is_ascii_digit()) { + issues.push(format!( + "reference `{key}`: pmid `{pmid}` is not a number. A `PMC...` id goes in \ + `pmcid`." + )); + } + } if let Some(label) = &reference.label { labels.entry(label.as_str()).or_default().push(key); } @@ -897,6 +1214,20 @@ impl CourseFile { issues.push(format!("lecture `{id}`: unknown unit `{u}`")); } } + for objective in &lec.teaches { + if !self.learning_objectives.contains_key(objective) { + if self.learning_targets.contains_key(objective) { + issues.push(format!( + "lecture `{id}`: `teaches` names the target `{objective}`, but it \ + registers objectives. A target is reached through its objective." + )); + } else { + issues.push(format!( + "lecture `{id}`: `teaches` names an unknown objective `{objective}`" + )); + } + } + } let mut seen: Vec<(&str, &str)> = Vec::new(); for (index, reading) in lec.readings.iter().enumerate() { issues.extend(self.reading_issues(id, index, reading, &mut seen)); @@ -1770,6 +2101,7 @@ impl CourseFile { date: None, unit: Some("u-intro".to_string()), slides_url: None, + teaches: Vec::new(), readings: Vec::new(), }, ); @@ -1825,6 +2157,7 @@ impl CourseFile { learning_targets: targets, references: BTreeMap::new(), stimuli: BTreeMap::new(), + origins: BTreeMap::new(), } } } @@ -1893,7 +2226,7 @@ course: term: Spring 2026 "#, ); - assert_eq!(c.schema_version, "1.0"); + assert_eq!(c.schema_version, SCHEMA_VERSION); assert_eq!(c.policy.options_per_item, 4); assert_eq!(c.course.slug(), "biosc-1540"); assert!(c.validate().is_empty()); @@ -2040,6 +2373,105 @@ learning_objectives: assert_eq!(reading.cite("kuriyan2013molecules", reference), "KKW §6.1"); } + #[test] + fn a_link_resolves_from_the_exact_location_outward() { + let mut reference = Reference { + title: "Basic local alignment search tool".into(), + kind: ReferenceKind::Article, + ..Reference::default() + }; + + // Nothing at all to link to. + assert_eq!(reference.href(None, None), None); + + // A DOI is a link to the work, which beats nothing. + reference.doi = Some("10.1016/S0022-2836(05)80360-2".into()); + assert_eq!( + reference.href(None, None).as_deref(), + Some("https://doi.org/10.1016/S0022-2836(05)80360-2") + ); + + // The work's own URL is more use than its identifier. + reference.url = Some("https://example.org/blast".into()); + assert_eq!( + reference.href(None, None).as_deref(), + Some("https://example.org/blast") + ); + + // A location inside the work beats the work. + reference.base_url = Some("https://example.org/blast/".into()); + assert_eq!( + reference.href(None, Some("/§2")).as_deref(), + Some("https://example.org/blast/§2") + ); + assert_eq!( + reference + .href(Some("https://example.org/exact"), Some("§2")) + .as_deref(), + Some("https://example.org/exact") + ); + } + + #[test] + fn identifiers_are_normalized_before_they_become_links() { + let doi_as_url = Reference { + title: "T".into(), + doi: Some("https://doi.org/10.1/x".into()), + ..Reference::default() + }; + assert_eq!( + doi_as_url.href(None, None).as_deref(), + Some("https://doi.org/10.1/x") + ); + + let preprint = Reference { + title: "T".into(), + kind: ReferenceKind::Preprint, + arxiv: Some("arXiv:2301.00001".into()), + ..Reference::default() + }; + assert_eq!( + preprint.href(None, None).as_deref(), + Some("https://arxiv.org/abs/2301.00001") + ); + + // A bare number is still a PMC id. + let open_access = Reference { + title: "T".into(), + pmcid: Some("3084216".into()), + ..Reference::default() + }; + assert_eq!( + open_access.href(None, None).as_deref(), + Some("https://www.ncbi.nlm.nih.gov/pmc/articles/PMC3084216/") + ); + } + + #[test] + fn something_that_is_not_a_doi_is_reported() { + let c = parse( + r#" +course: { code: X, title: Y, term: Z } +references: + bad: + title: A work + doi: nature12373 + worse: + title: Another work + pmid: PMC3084216 +"#, + ); + let issues = c.validate(); + assert!( + issues.iter().any(|i| i.contains("is not a DOI")), + "{issues:?}" + ); + assert!( + issues.iter().any(|i| i.contains("is not a number")), + "{issues:?}" + ); + } + #[test] fn a_bare_string_reading_still_parses_and_round_trips() { let c = parse( diff --git a/src/model/course/fragment.rs b/src/model/course/fragment.rs new file mode 100644 index 0000000..7a88885 --- /dev/null +++ b/src/model/course/fragment.rs @@ -0,0 +1,866 @@ +// SPDX-License-Identifier: Prosperity-3.0.0 +// Copyright Scientific Computing Studio +// Source: https://git.scient.ing/education/coursebank + +//! One course, several files. +//! +//! A course of forty lectures does not fit in a file anyone wants to scroll. So +//! the registries [`CourseFile`] holds may be spread across a directory and +//! merged on load: +//! +//! ```text +//! course.yaml course, policy, units +//! references.yaml references +//! lectures/l-1-2.yaml one lecture, its readings, and what it teaches +//! objectives/lo-x.yaml one objective and its targets +//! ``` +//! +//! The merge happens in memory on every command. Nothing is generated on disk +//! and no command depends on a build step, because a generated file that other +//! commands read is a file that goes stale. `coursebank course build` exists to +//! show you the merged result, and nothing reads what it writes. +//! +//! # What this does not relax +//! +//! Splitting a file is only worth doing if it cannot introduce a second +//! definition of the same thing. Two rules keep that true, and both are enforced +//! here rather than left to convention: +//! +//! * **One definition site per id.** Two files defining `lo-read-file-formats` +//! is an error naming both paths. The winner is not the last file loaded, +//! because there is no winner. +//! * **A section belongs to a kind of file.** A file under `lectures/` may not +//! define `learning_objectives`. Otherwise the layout decays into forty files +//! that each might hold anything, which is the same navigation problem in a +//! worse shape. +//! +//! `course.yaml` is exempt from the second rule: a course that has not been +//! split is a single fragment that happens to define everything, and it keeps +//! loading unchanged. +//! +//! # Two derivations +//! +//! Splitting by lecture makes two fields tedious to maintain by hand, so they +//! are derived instead: +//! +//! * A lecture's `teaches:` list adds that lecture to each named objective's +//! `lectures`. You write what a lecture covers while planning the lecture, +//! which is when you know. +//! * A target with no `lectures` of its own inherits its objective's, the same +//! way it already inherits `level_ceiling`. +//! +//! Both are unions and both are idempotent, so declaring a pair on both sides +//! is redundant rather than contradictory. + +use std::collections::BTreeMap; +use std::fmt; +use std::path::{Path, PathBuf}; + +use serde::{Deserialize, Serialize}; + +use super::{ + COURSE_FILE, Course, CourseFile, Lecture, Objective, Policy, Reference, SCHEMA_VERSION, + SUPPORTED_MAJORS, Stimulus, Target, Unit, +}; +use crate::error::{Error, Result}; +use crate::layout::Layout; +use crate::yaml; + +/// The file holding the bibliography when it is kept out of `course.yaml`. +pub const REFERENCES_FILE: &str = "references.yaml"; + +/// One registry section of a course. +/// +/// Used to say which file a fact came from, and to keep a fragment from +/// defining something that belongs somewhere else. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum Section { + /// Course identity. + Course, + /// Course-wide policy. + Policy, + /// Units. + Units, + /// Lectures. + Lectures, + /// Learning objectives. + Objectives, + /// Learning targets. + Targets, + /// Works the course cites. + References, + /// Shared stimuli. + Stimuli, +} + +impl Section { + /// Every section, in the order a merged course lists them. + pub const ALL: [Section; 8] = [ + Section::Course, + Section::Policy, + Section::Units, + Section::Lectures, + Section::Objectives, + Section::Targets, + Section::References, + Section::Stimuli, + ]; + + /// The YAML key this section is written under. + pub fn key(self) -> &'static str { + match self { + Section::Course => "course", + Section::Policy => "policy", + Section::Units => "units", + Section::Lectures => "lectures", + Section::Objectives => "learning_objectives", + Section::Targets => "learning_targets", + Section::References => "references", + Section::Stimuli => "stimuli", + } + } + + /// What one of its entries is called in a message. + pub fn noun(self) -> &'static str { + match self { + Section::Course => "course identity", + Section::Policy => "policy", + Section::Units => "unit", + Section::Lectures => "lecture", + Section::Objectives => "objective", + Section::Targets => "target", + Section::References => "reference", + Section::Stimuli => "stimulus", + } + } + + /// Where a file defining this section is expected to live. + pub fn home(self) -> &'static str { + match self { + Section::Course | Section::Policy | Section::Units => COURSE_FILE, + Section::Lectures => "lectures/*.yaml", + Section::Objectives | Section::Targets | Section::Stimuli => "objectives/*.yaml", + Section::References => REFERENCES_FILE, + } + } +} + +impl fmt::Display for Section { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.key()) + } +} + +/// What kind of file a fragment is, which fixes what it may define. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Role { + /// `course.yaml`. May define anything, so an unsplit course still loads. + Root, + /// `references.yaml`. + References, + /// A file under `lectures/`. + Lecture, + /// A file under `objectives/`. + Objective, +} + +impl Role { + /// Whether a file in this role may define a section. + pub fn allows(self, section: Section) -> bool { + match self { + Role::Root => true, + Role::References => section == Section::References, + Role::Lecture => section == Section::Lectures, + Role::Objective => matches!( + section, + Section::Objectives | Section::Targets | Section::Stimuli + ), + } + } + + /// A short name for a message. + pub fn label(self) -> &'static str { + match self { + Role::Root => "course", + Role::References => "references", + Role::Lecture => "lecture", + Role::Objective => "objective", + } + } +} + +/// One file's worth of course registries. +/// +/// Every section is optional, which is what makes this both the fragment schema +/// and — with every section filled in — the schema of an unsplit `course.yaml`. +/// [`CourseFile`] is the resolved model the rest of the crate reads; this is +/// only what one file on disk is allowed to say. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Fragment { + /// Schema version this file targets. + #[serde( + default, + deserialize_with = "yaml::flexible_string_opt", + skip_serializing_if = "Option::is_none" + )] + pub schema_version: Option, + + /// Course identity. Exactly one fragment must carry it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub course: Option, + + /// Course-wide policy. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub policy: Option, + + /// Units, in teaching order. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub units: Vec, + + /// Lectures by id. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub lectures: BTreeMap, + + /// Learning objectives by id. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub learning_objectives: BTreeMap, + + /// Learning targets by id. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub learning_targets: BTreeMap, + + /// Works the course cites, by citation key. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub references: BTreeMap, + + /// Shared stimuli by id. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub stimuli: BTreeMap, +} + +impl Fragment { + /// Loads one fragment from disk. + /// + /// # Arguments + /// + /// * `path` - the file to read. + /// + /// # Returns + /// + /// The parsed fragment. + /// + /// # Errors + /// + /// Returns [`Error::Io`] if unreadable and [`Error::Yaml`] if it does not + /// match the schema. Unknown keys are errors, so a misspelled section name + /// is caught here rather than silently contributing nothing. + pub fn load(path: &Path) -> Result { + yaml::read(path) + } + + /// Which sections this fragment actually defines. + pub fn sections(&self) -> Vec
{ + let mut out = Vec::new(); + if self.course.is_some() { + out.push(Section::Course); + } + if self.policy.is_some() { + out.push(Section::Policy); + } + if !self.units.is_empty() { + out.push(Section::Units); + } + if !self.lectures.is_empty() { + out.push(Section::Lectures); + } + if !self.learning_objectives.is_empty() { + out.push(Section::Objectives); + } + if !self.learning_targets.is_empty() { + out.push(Section::Targets); + } + if !self.references.is_empty() { + out.push(Section::References); + } + if !self.stimuli.is_empty() { + out.push(Section::Stimuli); + } + out + } +} + +/// The fragment files of a course directory, in load order, with their roles. +/// +/// `course.yaml` is listed whether or not it exists, so a directory that is not +/// a course fails with a message naming the file it wanted rather than an empty +/// merge. Directory contents are sorted, which is what makes the merged course +/// independent of filesystem order. +/// +/// # Arguments +/// +/// * `layout` - the resolved course layout. +/// +/// # Returns +/// +/// Paths paired with what each file is allowed to define. +/// +/// # Errors +/// +/// Returns [`Error::Io`] when a fragment directory exists but cannot be read. +pub fn files(layout: &Layout) -> Result> { + let mut out = vec![(layout.course_file(), Role::Root)]; + + let references = layout.references_file(); + if references.is_file() { + out.push((references, Role::References)); + } + for path in yaml::list_yaml(&layout.lectures())? { + out.push((path, Role::Lecture)); + } + for path in yaml::list_yaml(&layout.objectives())? { + out.push((path, Role::Objective)); + } + Ok(out) +} + +/// Loads every fragment in a course directory and merges them into one course. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// +/// # Returns +/// +/// The merged course, with [`CourseFile::origins`] recording which file defined +/// each id. +/// +/// # Errors +/// +/// Propagates load errors, and returns [`Error::Invalid`] with every merge +/// problem at once: an id defined twice, a section in the wrong kind of file, a +/// fragment written against another major schema version, or no `course:` +/// section anywhere. +/// +/// Cross-references are *not* checked here. A dangling objective id is a +/// content problem, and content problems are [`CourseFile::validate`]'s, so that +/// they are reported the same way whether or not the course is split. +pub fn assemble(root: &Path) -> Result { + let layout = Layout::new(root); + let mut merge = Merge::default(); + + for (path, role) in files(&layout)? { + let fragment = Fragment::load(&path)?; + let shown = path.strip_prefix(root).unwrap_or(&path).to_path_buf(); + merge.take(&shown, role, fragment); + } + + merge.resolve(); + merge.finish(root) +} + +/// Accumulates fragments, remembering where each id came from. +#[derive(Debug, Default)] +struct Merge { + schema_version: Option, + course: Option, + policy: Option, + units: Vec, + lectures: BTreeMap, + objectives: BTreeMap, + targets: BTreeMap, + references: BTreeMap, + stimuli: BTreeMap, + origins: BTreeMap<(Section, String), PathBuf>, + issues: Vec, +} + +impl Merge { + /// Folds one fragment in. + /// + /// # Arguments + /// + /// * `path` - the fragment's path relative to the course root, for messages. + /// * `role` - what this file is allowed to define. + /// * `fragment` - the parsed fragment. + fn take(&mut self, path: &Path, role: Role, fragment: Fragment) { + for section in fragment.sections() { + if !role.allows(section) { + self.issues.push(format!( + "{}: a {} file may not define `{}`; that section belongs in {}", + path.display(), + role.label(), + section.key(), + section.home() + )); + } + } + + if let Some(declared) = &fragment.schema_version { + if !SUPPORTED_MAJORS.contains(&major(declared)) { + self.issues.push(format!( + "{}: declares schema_version {declared}, which this build cannot read. It \ + writes {SCHEMA_VERSION} and reads {}.", + path.display(), + SUPPORTED_MAJORS + .iter() + .map(|m| format!("{m}.x")) + .collect::>() + .join(" and ") + )); + } + if self.schema_version.is_none() { + self.schema_version = Some(declared.clone()); + } + } + + if role.allows(Section::Course) { + if let Some(course) = fragment.course { + if self.claim(Section::Course, path) { + self.course = Some(course); + } + } + } + if role.allows(Section::Policy) { + if let Some(policy) = fragment.policy { + if self.claim(Section::Policy, path) { + self.policy = Some(policy); + } + } + } + + if role.allows(Section::Units) { + for unit in fragment.units { + let key = (Section::Units, unit.id.clone()); + if let Some(first) = self.origins.get(&key) { + let message = duplicate(Section::Units, &unit.id, first.as_path(), path); + self.issues.push(message); + continue; + } + self.origins.insert(key, path.to_path_buf()); + self.units.push(unit); + } + } + + if role.allows(Section::Lectures) { + absorb( + &mut self.lectures, + fragment.lectures, + Section::Lectures, + path, + &mut self.origins, + &mut self.issues, + ); + } + if role.allows(Section::Objectives) { + absorb( + &mut self.objectives, + fragment.learning_objectives, + Section::Objectives, + path, + &mut self.origins, + &mut self.issues, + ); + } + if role.allows(Section::Targets) { + absorb( + &mut self.targets, + fragment.learning_targets, + Section::Targets, + path, + &mut self.origins, + &mut self.issues, + ); + } + if role.allows(Section::References) { + absorb( + &mut self.references, + fragment.references, + Section::References, + path, + &mut self.origins, + &mut self.issues, + ); + } + if role.allows(Section::Stimuli) { + absorb( + &mut self.stimuli, + fragment.stimuli, + Section::Stimuli, + path, + &mut self.origins, + &mut self.issues, + ); + } + } + + /// Records a section that may only be declared once. + /// + /// # Arguments + /// + /// * `section` - the section being claimed. + /// * `path` - the file claiming it. + /// + /// # Returns + /// + /// Whether the claim was the first, and so whether the caller should store + /// what it parsed. + fn claim(&mut self, section: Section, path: &Path) -> bool { + let key = (section, String::new()); + if let Some(first) = self.origins.get(&key) { + let message = format!( + "`{}` is declared twice: {} and {}. It applies to the whole course, so it has \ + one definition site.", + section.key(), + first.display(), + path.display() + ); + self.issues.push(message); + return false; + } + self.origins.insert(key, path.to_path_buf()); + true + } + + /// Fills in the two fields a split layout would otherwise duplicate. + fn resolve(&mut self) { + // A lecture says what it teaches; the objective's lecture list follows. + for (lecture_id, lecture) in &self.lectures { + for objective_id in &lecture.teaches { + if let Some(objective) = self.objectives.get_mut(objective_id) { + if !objective.lectures.iter().any(|l| l == lecture_id) { + objective.lectures.push(lecture_id.clone()); + } + } + } + } + + // A target with no lecture of its own is taught wherever its objective + // is. Collected first: the read of `objectives` and the write to + // `targets` cannot overlap in one pass. + let inherited: Vec<(String, Vec)> = self + .targets + .iter() + .filter(|(_, target)| target.lectures.is_empty()) + .filter_map(|(id, target)| { + self.objectives + .get(&target.objective) + .map(|objective| (id.clone(), objective.lectures.clone())) + }) + .collect(); + for (id, lectures) in inherited { + if let Some(target) = self.targets.get_mut(&id) { + target.lectures = lectures; + } + } + } + + /// Builds the course, or reports every merge problem at once. + fn finish(self, root: &Path) -> Result { + let mut issues = self.issues; + let course = match self.course { + Some(course) => course, + None => { + issues.push(format!( + "no file in {} declares a `course:` section, so the course has no code, \ + title, or term", + root.display() + )); + return Err(Error::Invalid(issues)); + } + }; + if !issues.is_empty() { + return Err(Error::Invalid(issues)); + } + + Ok(CourseFile { + schema_version: self + .schema_version + .unwrap_or_else(|| SCHEMA_VERSION.to_string()), + course, + policy: self.policy.unwrap_or_default(), + units: self.units, + lectures: self.lectures, + learning_objectives: self.objectives, + learning_targets: self.targets, + references: self.references, + stimuli: self.stimuli, + origins: self.origins, + }) + } +} + +/// Moves one section's entries across, refusing a second definition. +fn absorb( + into: &mut BTreeMap, + from: BTreeMap, + section: Section, + path: &Path, + origins: &mut BTreeMap<(Section, String), PathBuf>, + issues: &mut Vec, +) { + for (id, value) in from { + let key = (section, id.clone()); + if let Some(first) = origins.get(&key) { + issues.push(duplicate(section, &id, first.as_path(), path)); + continue; + } + origins.insert(key, path.to_path_buf()); + into.insert(id, value); + } +} + +/// The message for an id defined in two files. +fn duplicate(section: Section, id: &str, first: &Path, second: &Path) -> String { + format!( + "{} `{id}` is defined in two places: {} and {}. An id has one definition site; delete \ + one or rename it.", + section.noun(), + first.display(), + second.display() + ) +} + +/// The part of a schema version before the first dot. +fn major(version: &str) -> &str { + version.split('.').next().unwrap_or(version) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn tmp(tag: &str) -> PathBuf { + let p = std::env::temp_dir().join(format!("coursebank-frag-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&p); + std::fs::create_dir_all(&p).unwrap(); + p + } + + fn write(root: &Path, relative: &str, body: &str) { + let path = root.join(relative); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, body).unwrap(); + } + + const ROOT: &str = r#" +course: + code: BIOSC 1540 + title: Computational Biology + term: 2026f +policy: + points_per_item: 1.0 +units: + - id: u1 + title: Search and Similarity +"#; + + #[test] + fn a_split_course_merges_into_one_model() { + let root = tmp("merge"); + write(&root, "course.yaml", ROOT); + write( + &root, + "references.yaml", + "references:\n ismail2023:\n title: Bioinformatics\n", + ); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: The Digital Genome\n unit: u1\n \ + teaches: [lo-read-file-formats]\n", + ); + write( + &root, + "objectives/lo-read-file-formats.yaml", + "learning_objectives:\n lo-read-file-formats:\n text: Read the text formats.\n \ + unit: u1\nlearning_targets:\n t-fastq-structure:\n text: Identify the four \ + lines.\n objective: lo-read-file-formats\n", + ); + + let course = assemble(&root).unwrap(); + assert_eq!(course.course.code, "BIOSC 1540"); + assert_eq!(course.units.len(), 1); + assert_eq!(course.references.len(), 1); + assert!(course.validate().is_empty(), "{:?}", course.validate()); + + // Derived: the lecture registered the objective, and the target + // inherited the objective's lecture. + assert_eq!( + course.learning_objectives["lo-read-file-formats"].lectures, + vec!["L1.2".to_string()] + ); + assert_eq!( + course.learning_targets["t-fastq-structure"].lectures, + vec!["L1.2".to_string()] + ); + assert_eq!(course.lecture_targets("L1.2"), vec!["t-fastq-structure"]); + } + + #[test] + fn an_unsplit_course_file_still_loads() { + let root = tmp("monolith"); + write( + &root, + "course.yaml", + &format!( + "{ROOT}lectures:\n L1.2:\n title: The Digital Genome\nlearning_objectives:\n \ + lo-x:\n text: Do the thing.\n lectures: [L1.2]\nlearning_targets:\n \ + t-x:\n text: Do the smaller thing.\n objective: lo-x\nreferences:\n \ + ismail2023:\n title: Bioinformatics\n" + ), + ); + + let course = assemble(&root).unwrap(); + assert!(course.validate().is_empty(), "{:?}", course.validate()); + assert_eq!(course.lecture_objectives("L1.2"), vec!["lo-x"]); + assert_eq!( + course.origin("lo-x").map(|(_, p)| p.to_path_buf()), + Some(PathBuf::from(COURSE_FILE)) + ); + } + + #[test] + fn an_id_defined_twice_names_both_files() { + let root = tmp("dup"); + write(&root, "course.yaml", ROOT); + let body = "learning_objectives:\n lo-x:\n text: Do the thing.\n"; + write(&root, "objectives/lo-x.yaml", body); + write(&root, "objectives/lo-x-old.yaml", body); + + let err = assemble(&root).unwrap_err(); + let message = err.to_string(); + assert!(message.contains("objectives/lo-x.yaml"), "{message}"); + assert!(message.contains("objectives/lo-x-old.yaml"), "{message}"); + assert!(message.contains("one definition site"), "{message}"); + } + + #[test] + fn a_section_in_the_wrong_kind_of_file_is_rejected() { + let root = tmp("misplaced"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: The Digital Genome\nlearning_objectives:\n lo-x:\n \ + text: Do the thing.\n", + ); + + let message = assemble(&root).unwrap_err().to_string(); + assert!( + message.contains("may not define `learning_objectives`"), + "{message}" + ); + assert!(message.contains("objectives/*.yaml"), "{message}"); + } + + #[test] + fn a_course_with_no_identity_says_so() { + let root = tmp("no-course"); + write(&root, "course.yaml", "units:\n - id: u1\n title: One\n"); + let message = assemble(&root).unwrap_err().to_string(); + assert!(message.contains("`course:` section"), "{message}"); + } + + #[test] + fn a_fragment_from_another_major_version_is_refused() { + let root = tmp("version"); + write(&root, "course.yaml", ROOT); + write( + &root, + "objectives/lo-x.yaml", + "schema_version: '9.0'\nlearning_objectives:\n lo-x:\n text: Do the thing.\n", + ); + let message = assemble(&root).unwrap_err().to_string(); + assert!(message.contains("schema_version 9.0"), "{message}"); + } + + #[test] + fn origins_point_at_the_fragment_that_defined_each_id() { + let root = tmp("origins"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: The Digital Genome\n", + ); + write( + &root, + "objectives/lo-x.yaml", + "learning_objectives:\n lo-x:\n text: Do the thing.\n", + ); + + let course = assemble(&root).unwrap(); + let (section, path) = course.origin("lo-x").unwrap(); + assert_eq!(section, Section::Objectives); + assert_eq!(path, Path::new("objectives/lo-x.yaml")); + let (section, path) = course.origin("L1.2").unwrap(); + assert_eq!(section, Section::Lectures); + assert_eq!(path, Path::new("lectures/l-1-2.yaml")); + assert!(course.origin("nothing-like-this").is_none()); + } + + #[test] + fn teaching_the_same_objective_from_two_lectures_unions() { + let root = tmp("union"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: One\n teaches: [lo-x]\n", + ); + write( + &root, + "lectures/l-1-3.yaml", + "lectures:\n L1.3:\n title: Two\n teaches: [lo-x]\n", + ); + write( + &root, + "objectives/lo-x.yaml", + "learning_objectives:\n lo-x:\n text: Do the thing.\n", + ); + + let course = assemble(&root).unwrap(); + assert_eq!( + course.learning_objectives["lo-x"].lectures, + vec!["L1.2".to_string(), "L1.3".to_string()] + ); + } + + #[test] + fn a_declaration_on_both_sides_is_not_duplicated() { + let root = tmp("both-sides"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: One\n teaches: [lo-x]\n", + ); + write( + &root, + "objectives/lo-x.yaml", + "learning_objectives:\n lo-x:\n text: Do the thing.\n lectures: [L1.2]\n", + ); + + let course = assemble(&root).unwrap(); + assert_eq!( + course.learning_objectives["lo-x"].lectures, + vec!["L1.2".to_string()] + ); + } + + #[test] + fn teaching_an_unknown_objective_is_a_validation_problem_not_a_merge_one() { + let root = tmp("unknown-teaches"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: One\n teaches: [lo-nope]\n", + ); + + let course = assemble(&root).unwrap(); + let issues = course.validate(); + assert!(issues.iter().any(|i| i.contains("lo-nope")), "{issues:?}"); + } +} diff --git a/src/model/item.rs b/src/model/item.rs index ae7e980..cf7ca4a 100644 --- a/src/model/item.rs +++ b/src/model/item.rs @@ -25,6 +25,7 @@ use serde::de::{self, MapAccess, Visitor}; use serde::ser::SerializeMap; use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use crate::course::Reference; use crate::date::Date; use crate::hash::fingerprint; use crate::taxonomy::{ @@ -44,9 +45,28 @@ pub struct Item { /// Revision counter, bumped whenever the content changes in a way that /// invalidates pooled statistics. - #[serde(default = "one_u32")] + /// Retained only so a pre-2.0 bank still loads. Ignored. + /// + /// A version number on a question answered the wrong question. It recorded + /// that *something* changed without constraining what, which meant an item + /// at version 3 might have a reworded distractor — fair, the statistics + /// still describe the same question — or a reworded stem, which makes it a + /// different question wearing the same id. Since 2.0 the stem *is* the + /// identity: reword it and you have a new item, with a new id and + /// [`Item::supersedes`] pointing back. [`Item::stem_digest`] is what + /// enforces that, against the seals of every administration. + /// + /// `coursebank migrate stems` removes it. + #[serde(default, skip_serializing)] pub version: u32, + /// The item this one replaces, when it is a rewording of an earlier stem. + /// + /// Lineage rather than versioning: both items stay in the bank, each with + /// its own statistics, and a report can say which one a cohort answered. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub supersedes: Option, + /// Workflow state; only [`Status::Approved`] items may be assembled. pub status: Status, @@ -145,7 +165,16 @@ pub struct Item { pub review: Option, /// Append-only change log. - #[serde(default, skip_serializing_if = "Vec::is_empty")] + /// Retained only so a pre-2.0 bank still loads. Ignored. + /// + /// A hand-maintained change log inside a version-controlled file, every + /// entry of which duplicated what `git log -p` already knew, with no + /// guarantee of agreeing with it. What git cannot express is a claim about + /// the item rather than a record of an edit, and that has its own fields: + /// [`Item::retired`] and [`Item::supersedes`]. + /// + /// `coursebank migrate stems` removes it. + #[serde(default, skip_serializing)] pub history: Vec, /// The author of record. @@ -170,7 +199,17 @@ pub struct Item { #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct Choice { - /// Option letter, `A` through `H`. + /// The option's id, unique within its item: `o-fourth-line`. + /// + /// A name rather than a position. Until 2.0 this was a letter, which put a + /// position in a field that pooled statistics, student feedback, and + /// `credit_overrides` all join on — so reordering a YAML block silently + /// moved the misconception recorded against one option onto another. The + /// letter a student sees is derived per form from the form's seed and lives + /// in the seal; see [`crate::seal::printed_letter`]. + /// + /// A single letter `A` through `H` still loads, so a bank migrates when you + /// run `coursebank migrate options` rather than when you upgrade. pub id: String, /// The option text. @@ -319,6 +358,19 @@ impl Citation { (None, None) => String::new(), } } + + /// The link for this location, resolved against the work it points into. + /// + /// # Arguments + /// + /// * `reference` - the work, looked up from the citation key. + /// + /// # Returns + /// + /// The most specific link available. See [`Reference::href`]. + pub fn href(&self, reference: &Reference) -> Option { + reference.href(self.url.as_deref(), self.path.as_deref()) + } } /// Writes a citation as a mapping, or as a bare string when that is all it holds. @@ -661,7 +713,7 @@ pub struct Retirement { #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct HistoryEntry { - /// The version this change produced. + /// The version this change produced. Ignored since 2.0. pub version: u32, /// When it was made. pub date: Date, @@ -672,6 +724,32 @@ pub struct HistoryEntry { pub change: String, } +/// The separator a pre-2.0 bank-qualified item id used: `b-1-2::q-fastq-line`. +pub const LEGACY_QUALIFIER: &str = "::"; + +/// An item id with any pre-2.0 bank qualifier removed. +/// +/// Until 2.0 an item was named `bank::item`, which made the file it happened to +/// live in part of its identity — and therefore part of the join key on every +/// row of response data ever collected. Moving a question between banks renamed +/// it. Since 2.0 the id names the item course-wide and the bank is only where it +/// is kept, so anything reading an old id strips the qualifier rather than +/// failing to match. +/// +/// # Arguments +/// +/// * `id` - an item id in either form. +/// +/// # Returns +/// +/// The part after the qualifier, or the whole id when there is none. +pub fn canonical_id(id: &str) -> &str { + match id.split_once(LEGACY_QUALIFIER) { + Some((_, rest)) => rest, + None => id, + } +} + impl Item { /// Builds a draft item with everything optional left empty. /// @@ -725,6 +803,7 @@ impl Item { author: None, notes_private: None, retired: None, + supersedes: None, } } @@ -758,19 +837,38 @@ impl Item { out } - /// Looks up an option by letter. + /// Looks up an option by id. /// /// # Arguments /// - /// * `letter` - the option id, case insensitive. + /// * `id` - the option id. A pre-2.0 letter matches case-insensitively, + /// which a slug never needs but a hand-typed `d` does. /// /// # Returns /// /// The option, or `None`. - pub fn option(&self, letter: &str) -> Option<&Choice> { + pub fn option(&self, id: &str) -> Option<&Choice> { self.options .iter() - .find(|o| o.id.eq_ignore_ascii_case(letter)) + .find(|o| o.id == id) + .or_else(|| self.options.iter().find(|o| o.id.eq_ignore_ascii_case(id))) + } + + /// Whether an option id is a pre-2.0 letter rather than a name. + /// + /// # Arguments + /// + /// * `id` - the option id. + /// + /// # Returns + /// + /// `true` for `A` through `H`. + pub fn is_legacy_option_id(id: &str) -> bool { + id.len() == 1 + && id + .chars() + .next() + .is_some_and(|c| c.is_ascii_uppercase() && c <= 'H') } /// Whether the item keys more than one option. @@ -834,6 +932,29 @@ impl Item { fingerprint(parts.iter().map(|s| s.as_str())) } + /// A digest of what the item asks, without its options. + /// + /// The identity check. [`Item::fingerprint`] covers the options too, which + /// is right for calibration — reword a distractor and the pooled selection + /// rates no longer describe what students saw — but wrong for identity, + /// because a question whose distractors changed is still the same question. + /// This covers the stem and the stimulus, and nothing else. + /// + /// Compared against the digest each seal recorded, which is what makes + /// "a reworded stem is a new stem" a rule the tool enforces rather than a + /// convention that decays. + /// + /// # Returns + /// + /// The digest as hex. + pub fn stem_digest(&self) -> String { + let mut parts: Vec = vec![self.stem.trim().to_string()]; + if let Some(s) = &self.stimulus { + parts.push(format!("stimulus:{s}")); + } + fingerprint(parts.iter().map(|s| s.as_str())) + } + /// Whether the recorded calibration matches the current content. /// /// # Returns @@ -889,16 +1010,20 @@ impl Item { } } - /// Appends a change-log entry and bumps the version. + /// Appends a change-log entry. + /// + /// Kept for the pre-2.0 banks that still carry a `history:` block, so + /// reading one and writing it back does not silently drop entries. New + /// entries belong in a commit message. /// /// # Arguments /// /// * `change` - a description of what changed. /// * `author` - who made the change. pub fn record_change(&mut self, change: &str, author: Option<&str>) { - self.version += 1; + let version = self.history.iter().map(|h| h.version).max().unwrap_or(0) + 1; self.history.push(HistoryEntry { - version: self.version, + version, date: Date::today(), author: author.map(|a| a.to_string()), change: change.to_string(), @@ -906,9 +1031,6 @@ impl Item { } } -fn one_u32() -> u32 { - 1 -} fn default_format() -> Format { Format::SingleBestAnswer } @@ -938,7 +1060,6 @@ options: #[test] fn minimal_item_parses_with_defaults() { let it = item(MINIMAL); - assert_eq!(it.version, 1); assert_eq!(it.format, Format::SingleBestAnswer); assert!(!it.bonus); assert_eq!(it.key_letters(), vec!["A"]); @@ -1053,12 +1174,24 @@ options: } #[test] - fn record_change_bumps_version_and_logs() { + fn the_stem_is_the_identity_rather_than_a_version_number() { let mut it = item(MINIMAL); + let before = it.stem_digest(); + + // A change log entry numbers itself and leaves the item alone: since + // 2.0 nothing reads `version`, and rewording a stem is not a version + // bump but a new item. it.record_change("clarified the stem", Some("Alex")); - assert_eq!(it.version, 2); + assert_eq!(it.version, 0); assert_eq!(it.history.len(), 1); - assert_eq!(it.history[0].version, 2); + assert_eq!(it.history[0].version, 1); + assert_eq!(it.stem_digest(), before); + + // The options are the fingerprint's business, not the stem's. + it.options[1].text = "a different distractor".into(); + assert_eq!(it.stem_digest(), before); + it.stem = "What is y?".into(); + assert_ne!(it.stem_digest(), before); } #[test] diff --git a/src/model/layout.rs b/src/model/layout.rs index ddc0c24..16772bc 100644 --- a/src/model/layout.rs +++ b/src/model/layout.rs @@ -3,6 +3,11 @@ // Source: https://git.scient.ing/education/coursebank //! The on-disk layout of a course directory. +//! +//! Two of these directories hold fragments of the course file rather than files +//! of their own kind: `lectures/` and `objectives/` are merged into one +//! [`crate::course::CourseFile`] on load, along with `references.yaml`. See +//! [`crate::course::fragment`]. use std::path::PathBuf; @@ -35,6 +40,25 @@ impl Layout { self.root.join(COURSE_FILE) } + /// Path to `references.yaml`, the bibliography when it is kept out of + /// `course.yaml`. + /// + /// Optional: absent means the course keeps its `references:` section in the + /// course file, which is how an unsplit course is arranged. + pub fn references_file(&self) -> PathBuf { + self.root.join(crate::course::fragment::REFERENCES_FILE) + } + + /// Directory holding one file per lecture. + pub fn lectures(&self) -> PathBuf { + self.root.join("lectures") + } + + /// Directory holding one file per learning objective. + pub fn objectives(&self) -> PathBuf { + self.root.join("objectives") + } + /// Directory holding item bank YAML files. pub fn banks(&self) -> PathBuf { self.root.join("banks") @@ -93,6 +117,8 @@ impl Layout { pub fn create_all(&self) -> Result<()> { for dir in [ self.root.clone(), + self.lectures(), + self.objectives(), self.banks(), self.assessments(), self.data(), diff --git a/src/model/seal.rs b/src/model/seal.rs index b5c9236..0b5f5cc 100644 --- a/src/model/seal.rs +++ b/src/model/seal.rs @@ -145,8 +145,16 @@ pub struct SealedItem { /// The item's global id. pub item: String, /// The item version as administered. + /// Retained only so a pre-2.0 seal still loads. Ignored when reasoning + /// about the item, but still written: [`SealFile::digest_input`] covers it, + /// so dropping it on a round trip would invalidate the digest of every + /// administration sealed before 2.0. #[serde(default, skip_serializing_if = "Option::is_none")] pub version: Option, + + /// The stem's digest as administered. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub stem_digest: Option, /// The item's content fingerprint, the same one /// [`crate::item::Item::fingerprint`] computes, so a seal and a bank can be /// compared without re-hashing either by hand. @@ -484,7 +492,8 @@ fn sealed_item( SealedItem { number: placement.number, item: placement.item.clone(), - version: placement.version.or(Some(item.version)), + version: None, + stem_digest: Some(item.stem_digest()), fingerprint: item.fingerprint(), points: placement .points @@ -638,6 +647,33 @@ impl SealFile { yaml::read(path) } + /// Loads every seal in a directory, oldest administration first. + /// + /// # Arguments + /// + /// * `dir` - the seals directory. + /// + /// # Returns + /// + /// The seals, empty when the directory does not exist. + /// + /// # Errors + /// + /// Propagates load failures, including a seal that does not parse. + pub fn load_all(dir: &Path) -> Result> { + let mut out = Vec::new(); + for path in crate::yaml::list_yaml(dir)? { + out.push(SealFile::load(&path)?); + } + out.sort_by(|a, b| { + a.seal + .date + .cmp(&b.seal.date) + .then(a.seal.assessment.cmp(&b.seal.assessment)) + }); + Ok(out) + } + /// Loads the seal for an assessment, if one has been written. /// /// Absence is not an error. A course that has never sealed anything should @@ -719,6 +755,12 @@ impl SealFile { self.schema_version, self.seal.assessment, self.seal.course, self.seal.term )); for item in &self.items { + // Appended only when present: a seal written before 2.0 has to keep + // producing the input it was digested from, or every older + // administration fails verification. + if let Some(stem) = &item.stem_digest { + buf.push_str(&format!("stem\u{1f}{}\u{1f}{stem}\n", item.number)); + } buf.push_str(&format!( "item\u{1f}{}\u{1f}{}\u{1f}{}\u{1f}{}\u{1f}{}\u{1f}{}\u{1f}{}\u{1f}{}\u{1f}", item.number, @@ -1157,6 +1199,7 @@ mod tests { number: 1, item: "b::q-1".into(), version: Some(1), + stem_digest: None, fingerprint: "abc".into(), points: 1.0, bonus: false, diff --git a/src/util/yaml.rs b/src/util/yaml.rs index 4e801ac..da97e63 100644 --- a/src/util/yaml.rs +++ b/src/util/yaml.rs @@ -16,7 +16,7 @@ use std::fs; use std::path::Path; use serde::de::{self, DeserializeOwned, Visitor}; -use serde::{Deserializer, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; use crate::error::{Error, Result}; @@ -61,6 +61,26 @@ pub fn write(path: &Path, value: &T) -> Result<()> { fs::write(path, text).map_err(|e| Error::io(path, e)) } +/// Serializes a value to a YAML string. +/// +/// Used where the caller needs to put something in front of the document, such +/// as the banner on a generated file. +/// +/// # Arguments +/// +/// * `value` - the value to serialize. +/// +/// # Returns +/// +/// The YAML text. +/// +/// # Errors +/// +/// Returns [`Error::Other`] if the value cannot be represented as YAML. +pub fn to_string(value: &T) -> Result { + serde_yaml_ng::to_string(value).map_err(Error::other) +} + /// Deserializes a JSON file into any type. /// /// Used only for importing legacy banks and for reading emitted schemas back in @@ -202,6 +222,33 @@ where d.deserialize_any(V) } +/// Deserializes an optional scalar as a string, quoted or not. +/// +/// The [`flexible_string`] of a field that may be absent, which is what a +/// fragment's `schema_version` is: one file in a course declares it and the +/// rest inherit. +/// +/// # Arguments +/// +/// * `d` - the deserializer. +/// +/// # Returns +/// +/// The value as a string, or `None`. +/// +/// # Errors +/// +/// Returns a deserialization error for non-scalar input. +pub fn flexible_string_opt<'de, D>(d: D) -> std::result::Result, D::Error> +where + D: Deserializer<'de>, +{ + #[derive(serde::Deserialize)] + struct Wrapper(#[serde(deserialize_with = "flexible_string")] String); + + Ok(Option::::deserialize(d)?.map(|w| w.0)) +} + #[cfg(test)] mod tests { use super::*;