From e680b098ec3b46592fae89a01e202dab8d6489f0 Mon Sep 17 00:00:00 2001 From: Ed L Date: Tue, 24 Mar 2026 07:32:48 -0700 Subject: [PATCH] refactor: remove Axis::Unassigned; axis_of returns Option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Axis::Unassigned served two purposes that Option already covers: 1. "this category has no assignment yet" → None 2. "this category doesn't exist" → None By removing the variant and changing axis_of to return Option, callers are forced by the compiler to handle the absent-category case explicitly (via match or unwrap_or), rather than silently treating it like a real axis value. SetAxis { axis: String } also upgraded to SetAxis { axis: Axis }. Previously, constructing SetAxis with an invalid string (e.g. "diagonal") would compile and then silently fail at dispatch. Now the type only admits valid axis values; the dispatch string-parser is gone. Axis gains #[serde(rename_all = "lowercase")] so existing JSON command files (smoke.jsonl, etc.) using "row"/"column"/"page" continue to work. Co-Authored-By: Claude Sonnet 4.6 --- src/command/dispatch.rs | 9 +------- src/command/types.rs | 4 ++-- src/model/model.rs | 28 ++++++++++++------------- src/ui/app.rs | 6 +++--- src/ui/category_panel.rs | 16 +++++++------- src/ui/tile_bar.rs | 16 +++++++------- src/view/axis.rs | 8 +++---- src/view/view.rs | 45 ++++++++++++++++++++-------------------- 8 files changed, 61 insertions(+), 71 deletions(-) diff --git a/src/command/dispatch.rs b/src/command/dispatch.rs index efa43cb..9debaa7 100644 --- a/src/command/dispatch.rs +++ b/src/command/dispatch.rs @@ -2,7 +2,6 @@ use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; use crate::formula::parse_formula; -use crate::view::Axis; use crate::persistence; use crate::import::analyzer::{analyze_records, extract_array_at_path, FieldKind}; use super::types::{CellValueArg, Command, CommandResult}; @@ -105,14 +104,8 @@ pub fn dispatch(model: &mut Model, cmd: &Command) -> CommandResult { } Command::SetAxis { category, axis } => { - let ax = match axis.to_lowercase().as_str() { - "row" | "rows" => Axis::Row, - "column" | "col" | "columns" => Axis::Column, - "page" | "pages" | "filter" => Axis::Page, - other => return CommandResult::err(format!("Unknown axis '{other}'")), - }; match model.active_view_mut() { - Some(view) => { view.set_axis(category, ax); CommandResult::ok() } + Some(view) => { view.set_axis(category, *axis); CommandResult::ok() } None => CommandResult::err("No active view"), } } diff --git a/src/command/types.rs b/src/command/types.rs index 283c8f6..ca7c6c0 100644 --- a/src/command/types.rs +++ b/src/command/types.rs @@ -1,4 +1,5 @@ use serde::{Deserialize, Serialize}; +use crate::view::Axis; /// All commands that can mutate a Model. /// @@ -44,8 +45,7 @@ pub enum Command { SwitchView { name: String }, /// Set the axis of a category in the active view. - /// axis: "row" | "column" | "page" - SetAxis { category: String, axis: String }, + SetAxis { category: String, axis: Axis }, /// Set the page-axis selection for a category. SetPageSelection { category: String, item: String }, diff --git a/src/model/model.rs b/src/model/model.rs index 3efda80..33d00a9 100644 --- a/src/model/model.rs +++ b/src/model/model.rs @@ -322,7 +322,7 @@ mod model_tests { fn add_category_notifies_existing_views() { let mut m = Model::new("Test"); m.add_category("Region").unwrap(); - assert_ne!(m.active_view().unwrap().axis_of("Region"), Axis::Unassigned); + assert_ne!(m.active_view().unwrap().axis_of("Region"), None); } #[test] @@ -378,8 +378,8 @@ mod model_tests { m.add_category("Product").unwrap(); m.create_view("Secondary"); let v = m.views.get("Secondary").unwrap(); - assert_ne!(v.axis_of("Region"), Axis::Unassigned); - assert_ne!(v.axis_of("Product"), Axis::Unassigned); + assert_ne!(v.axis_of("Region"), None); + assert_ne!(v.axis_of("Product"), None); } #[test] @@ -426,9 +426,9 @@ mod model_tests { m.add_category("Product").unwrap(); m.add_category("Time").unwrap(); let v = m.active_view().unwrap(); - assert_eq!(v.axis_of("Region"), Axis::Row); - assert_eq!(v.axis_of("Product"), Axis::Column); - assert_eq!(v.axis_of("Time"), Axis::Page); + assert_eq!(v.axis_of("Region"), Some(Axis::Row)); + assert_eq!(v.axis_of("Product"), Some(Axis::Column)); + assert_eq!(v.axis_of("Time"), Some(Axis::Page)); } #[test] @@ -968,11 +968,11 @@ mod five_category { fn default_view_first_two_on_axes_rest_on_page() { let m = build_model(); let v = m.active_view().unwrap(); - assert_eq!(v.axis_of("Region"), Axis::Row); - assert_eq!(v.axis_of("Product"), Axis::Column); - assert_eq!(v.axis_of("Channel"), Axis::Page); - assert_eq!(v.axis_of("Time"), Axis::Page); - assert_eq!(v.axis_of("Measure"), Axis::Page); + assert_eq!(v.axis_of("Region"), Some(Axis::Row)); + assert_eq!(v.axis_of("Product"), Some(Axis::Column)); + assert_eq!(v.axis_of("Channel"), Some(Axis::Page)); + assert_eq!(v.axis_of("Time"), Some(Axis::Page)); + assert_eq!(v.axis_of("Measure"), Some(Axis::Page)); } #[test] @@ -999,9 +999,9 @@ mod five_category { v.set_axis("Product", Axis::Page); v.set_axis("Measure", Axis::Page); } - assert_eq!(m.views.get("Default").unwrap().axis_of("Region"), Axis::Row); - assert_eq!(m.views.get("Pivot").unwrap().axis_of("Time"), Axis::Row); - assert_eq!(m.views.get("Pivot").unwrap().axis_of("Channel"), Axis::Column); + assert_eq!(m.views.get("Default").unwrap().axis_of("Region"), Some(Axis::Row)); + assert_eq!(m.views.get("Pivot").unwrap().axis_of("Time"), Some(Axis::Row)); + assert_eq!(m.views.get("Pivot").unwrap().axis_of("Channel"), Some(Axis::Column)); } #[test] diff --git a/src/ui/app.rs b/src/ui/app.rs index 9f8d55c..6f8fbe3 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -813,21 +813,21 @@ impl App { } KeyCode::Char('r') => { if let Some(name) = cat_names.get(cat_idx) { - command::dispatch(&mut self.model, &Command::SetAxis { category: name.clone(), axis: "row".to_string() }); + command::dispatch(&mut self.model, &Command::SetAxis { category: name.clone(), axis: Axis::Row }); self.dirty = true; } self.mode = AppMode::Normal; } KeyCode::Char('c') => { if let Some(name) = cat_names.get(cat_idx) { - command::dispatch(&mut self.model, &Command::SetAxis { category: name.clone(), axis: "column".to_string() }); + command::dispatch(&mut self.model, &Command::SetAxis { category: name.clone(), axis: Axis::Column }); self.dirty = true; } self.mode = AppMode::Normal; } KeyCode::Char('p') => { if let Some(name) = cat_names.get(cat_idx) { - command::dispatch(&mut self.model, &Command::SetAxis { category: name.clone(), axis: "page".to_string() }); + command::dispatch(&mut self.model, &Command::SetAxis { category: name.clone(), axis: Axis::Page }); self.dirty = true; } self.mode = AppMode::Normal; diff --git a/src/ui/category_panel.rs b/src/ui/category_panel.rs index 107c373..3b6f33e 100644 --- a/src/ui/category_panel.rs +++ b/src/ui/category_panel.rs @@ -67,16 +67,16 @@ impl<'a> Widget for CategoryPanel<'a> { let axis = view.axis_of(cat_name); let axis_str = match axis { - Axis::Row => "Row ↕", - Axis::Column => "Col ↔", - Axis::Page => "Page ☰", - Axis::Unassigned => "none", + Some(Axis::Row) => "Row ↕", + Some(Axis::Column) => "Col ↔", + Some(Axis::Page) => "Page ☰", + None => "none", }; let axis_color = match axis { - Axis::Row => Color::Green, - Axis::Column => Color::Blue, - Axis::Page => Color::Magenta, - Axis::Unassigned => Color::DarkGray, + Some(Axis::Row) => Color::Green, + Some(Axis::Column) => Color::Blue, + Some(Axis::Page) => Color::Magenta, + None => Color::DarkGray, }; let item_count = self.model.category(cat_name).map(|c| c.items.len()).unwrap_or(0); diff --git a/src/ui/tile_bar.rs b/src/ui/tile_bar.rs index d7523e5..a50dabc 100644 --- a/src/ui/tile_bar.rs +++ b/src/ui/tile_bar.rs @@ -41,10 +41,10 @@ impl<'a> Widget for TileBar<'a> { for (i, cat_name) in cat_names.iter().enumerate() { let axis = view.axis_of(cat_name); let axis_symbol = match axis { - Axis::Row => "↕", - Axis::Column => "↔", - Axis::Page => "☰", - Axis::Unassigned => "─", + Some(Axis::Row) => "↕", + Some(Axis::Column) => "↔", + Some(Axis::Page) => "☰", + None => "─", }; let label = format!(" [{cat_name} {axis_symbol}] "); @@ -54,10 +54,10 @@ impl<'a> Widget for TileBar<'a> { Style::default().fg(Color::Black).bg(Color::Cyan).add_modifier(Modifier::BOLD) } else { match axis { - Axis::Row => Style::default().fg(Color::Green), - Axis::Column => Style::default().fg(Color::Blue), - Axis::Page => Style::default().fg(Color::Magenta), - Axis::Unassigned => Style::default().fg(Color::DarkGray), + Some(Axis::Row) => Style::default().fg(Color::Green), + Some(Axis::Column) => Style::default().fg(Color::Blue), + Some(Axis::Page) => Style::default().fg(Color::Magenta), + None => Style::default().fg(Color::DarkGray), } }; diff --git a/src/view/axis.rs b/src/view/axis.rs index 369a4ac..42e6ef7 100644 --- a/src/view/axis.rs +++ b/src/view/axis.rs @@ -1,21 +1,19 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] pub enum Axis { Row, Column, Page, - /// Not yet assigned - Unassigned, } impl std::fmt::Display for Axis { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Axis::Row => write!(f, "Row ↕"), + Axis::Row => write!(f, "Row ↕"), Axis::Column => write!(f, "Col ↔"), - Axis::Page => write!(f, "Page ☰"), - Axis::Unassigned => write!(f, "─"), + Axis::Page => write!(f, "Page ☰"), } } } diff --git a/src/view/view.rs b/src/view/view.rs index d123336..f5340bb 100644 --- a/src/view/view.rs +++ b/src/view/view.rs @@ -61,8 +61,8 @@ impl View { } } - pub fn axis_of(&self, cat_name: &str) -> Axis { - self.category_axes.get(cat_name).copied().unwrap_or(Axis::Unassigned) + pub fn axis_of(&self, cat_name: &str) -> Option { + self.category_axes.get(cat_name).copied() } pub fn categories_on(&self, axis: Axis) -> Vec<&str> { @@ -72,6 +72,7 @@ impl View { .collect() } + pub fn set_page_selection(&mut self, cat_name: &str, item: &str) { self.page_selections.insert(cat_name.to_string(), item.to_string()); } @@ -112,12 +113,10 @@ impl View { /// Cycle axis for a category: Row → Column → Page → Row pub fn cycle_axis(&mut self, cat_name: &str) { - let current = self.axis_of(cat_name); - let next = match current { - Axis::Row => Axis::Column, - Axis::Column => Axis::Page, - Axis::Page => Axis::Row, - Axis::Unassigned => Axis::Row, + let next = match self.axis_of(cat_name) { + Some(Axis::Row) | None => Axis::Column, + Some(Axis::Column) => Axis::Page, + Some(Axis::Page) => Axis::Row, }; self.set_axis(cat_name, next); self.selected = (0, 0); @@ -140,27 +139,27 @@ mod tests { #[test] fn first_category_assigned_to_row() { let v = view_with_cats(&["Region"]); - assert_eq!(v.axis_of("Region"), Axis::Row); + assert_eq!(v.axis_of("Region"), Some(Axis::Row)); } #[test] fn second_category_assigned_to_column() { let v = view_with_cats(&["Region", "Product"]); - assert_eq!(v.axis_of("Product"), Axis::Column); + assert_eq!(v.axis_of("Product"), Some(Axis::Column)); } #[test] fn third_and_later_categories_assigned_to_page() { let v = view_with_cats(&["Region", "Product", "Time", "Scenario"]); - assert_eq!(v.axis_of("Time"), Axis::Page); - assert_eq!(v.axis_of("Scenario"), Axis::Page); + assert_eq!(v.axis_of("Time"), Some(Axis::Page)); + assert_eq!(v.axis_of("Scenario"), Some(Axis::Page)); } #[test] fn set_axis_changes_assignment() { let mut v = view_with_cats(&["Region", "Product"]); v.set_axis("Region", Axis::Column); - assert_eq!(v.axis_of("Region"), Axis::Column); + assert_eq!(v.axis_of("Region"), Some(Axis::Column)); } #[test] @@ -172,9 +171,9 @@ mod tests { } #[test] - fn axis_of_unknown_category_returns_unassigned() { + fn axis_of_unknown_category_returns_none() { let v = View::new("Test"); - assert_eq!(v.axis_of("Ghost"), Axis::Unassigned); + assert_eq!(v.axis_of("Ghost"), None); } #[test] @@ -209,7 +208,7 @@ mod tests { fn cycle_axis_row_to_column() { let mut v = view_with_cats(&["Region", "Product"]); v.cycle_axis("Region"); - assert_eq!(v.axis_of("Region"), Axis::Column); + assert_eq!(v.axis_of("Region"), Some(Axis::Column)); } #[test] @@ -217,14 +216,14 @@ mod tests { let mut v = view_with_cats(&["Region", "Product"]); v.set_axis("Product", Axis::Column); v.cycle_axis("Product"); - assert_eq!(v.axis_of("Product"), Axis::Page); + assert_eq!(v.axis_of("Product"), Some(Axis::Page)); } #[test] fn cycle_axis_page_to_row() { let mut v = view_with_cats(&["Region", "Product", "Time"]); v.cycle_axis("Time"); - assert_eq!(v.axis_of("Time"), Axis::Row); + assert_eq!(v.axis_of("Time"), Some(Axis::Row)); } #[test] @@ -260,9 +259,9 @@ mod prop_tests { for c in &cats { v.on_category_added(c); } for c in &cats { let axis = v.axis_of(c); - prop_assert_ne!(axis, Axis::Unassigned, + prop_assert!(axis.is_some(), "category '{}' should be assigned after on_category_added", c); - let on_axis = v.categories_on(axis); + let on_axis = v.categories_on(axis.unwrap()); prop_assert!(on_axis.contains(&c.as_str()), "categories_on({:?}) should contain '{}'", axis, c); } @@ -288,9 +287,9 @@ mod prop_tests { fn on_category_added_idempotent(cats in unique_cat_names()) { let mut v = View::new("T"); for c in &cats { v.on_category_added(c); } - let axes_before: Vec<_> = cats.iter().map(|c| v.axis_of(c)).collect(); + let axes_before: Vec> = cats.iter().map(|c| v.axis_of(c)).collect(); for c in &cats { v.on_category_added(c); } - let axes_after: Vec<_> = cats.iter().map(|c| v.axis_of(c)).collect(); + let axes_after: Vec> = cats.iter().map(|c| v.axis_of(c)).collect(); prop_assert_eq!(axes_before, axes_after); } @@ -306,7 +305,7 @@ mod prop_tests { let idx = target_idx % cats.len(); let cat = &cats[idx]; v.set_axis(cat, axis); - prop_assert_eq!(v.axis_of(cat), axis); + prop_assert_eq!(v.axis_of(cat), Some(axis)); } /// After set_axis(cat, X), cat is NOT in categories_on(Y) for Y ≠ X