fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor: remove Axis::Unassigned; axis_of returns Option<Axis>

Browse Source 
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<Axis>,
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 <noreply@anthropic.com>
This commit is contained in:
Ed L
2026-03-24 07:32:48 -07:00
parent 599f1adcbd
commit e680b098ec
8 changed files with 61 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/command/dispatch.rs
View File

@ -2,7 +2,6 @@
use crate::model::Model; use crate::model::Model;
use crate::model::cell::{CellKey, CellValue}; use crate::model::cell::{CellKey, CellValue};
use crate::formula::parse_formula; use crate::formula::parse_formula;
use crate::view::Axis;
use crate::persistence; use crate::persistence;
use crate::import::analyzer::{analyze_records, extract_array_at_path, FieldKind}; use crate::import::analyzer::{analyze_records, extract_array_at_path, FieldKind};
use super::types::{CellValueArg, Command, CommandResult}; use super::types::{CellValueArg, Command, CommandResult};
@ -105,14 +104,8 @@ pub fn dispatch(model: &mut Model, cmd: &Command) -> CommandResult {
} }
Command::SetAxis { category, axis } => { 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() { 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"), None => CommandResult::err("No active view"),
} }
} }

4
src/command/types.rs
View File

@ -1,4 +1,5 @@
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use crate::view::Axis;
/// All commands that can mutate a Model. /// All commands that can mutate a Model.
/// ///
@ -44,8 +45,7 @@ pub enum Command {
SwitchView { name: String }, SwitchView { name: String },
/// Set the axis of a category in the active view. /// Set the axis of a category in the active view.
/// axis: "row" | "column" | "page" SetAxis { category: String, axis: Axis },
SetAxis { category: String, axis: String },
/// Set the page-axis selection for a category. /// Set the page-axis selection for a category.
SetPageSelection { category: String, item: String }, SetPageSelection { category: String, item: String },

28
src/model/model.rs
View File

@ -322,7 +322,7 @@ mod model_tests {
fn add_category_notifies_existing_views() { fn add_category_notifies_existing_views() {
let mut m = Model::new("Test"); let mut m = Model::new("Test");
m.add_category("Region").unwrap(); 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] #[test]
@ -378,8 +378,8 @@ mod model_tests {
m.add_category("Product").unwrap(); m.add_category("Product").unwrap();
m.create_view("Secondary"); m.create_view("Secondary");
let v = m.views.get("Secondary").unwrap(); let v = m.views.get("Secondary").unwrap();
assert_ne!(v.axis_of("Region"), Axis::Unassigned); assert_ne!(v.axis_of("Region"), None);
assert_ne!(v.axis_of("Product"), Axis::Unassigned); assert_ne!(v.axis_of("Product"), None);
} }
#[test] #[test]
@ -426,9 +426,9 @@ mod model_tests {
m.add_category("Product").unwrap(); m.add_category("Product").unwrap();
m.add_category("Time").unwrap(); m.add_category("Time").unwrap();
let v = m.active_view().unwrap(); let v = m.active_view().unwrap();
assert_eq!(v.axis_of("Region"), Axis::Row); assert_eq!(v.axis_of("Region"), Some(Axis::Row));
assert_eq!(v.axis_of("Product"), Axis::Column); assert_eq!(v.axis_of("Product"), Some(Axis::Column));
assert_eq!(v.axis_of("Time"), Axis::Page); assert_eq!(v.axis_of("Time"), Some(Axis::Page));
} }
#[test] #[test]
@ -968,11 +968,11 @@ mod five_category {
fn default_view_first_two_on_axes_rest_on_page() { fn default_view_first_two_on_axes_rest_on_page() {
let m = build_model(); let m = build_model();
let v = m.active_view().unwrap(); let v = m.active_view().unwrap();
assert_eq!(v.axis_of("Region"), Axis::Row); assert_eq!(v.axis_of("Region"), Some(Axis::Row));
assert_eq!(v.axis_of("Product"), Axis::Column); assert_eq!(v.axis_of("Product"), Some(Axis::Column));
assert_eq!(v.axis_of("Channel"), Axis::Page); assert_eq!(v.axis_of("Channel"), Some(Axis::Page));
assert_eq!(v.axis_of("Time"), Axis::Page); assert_eq!(v.axis_of("Time"), Some(Axis::Page));
assert_eq!(v.axis_of("Measure"), Axis::Page); assert_eq!(v.axis_of("Measure"), Some(Axis::Page));
} }
#[test] #[test]
@ -999,9 +999,9 @@ mod five_category {
v.set_axis("Product", Axis::Page); v.set_axis("Product", Axis::Page);
v.set_axis("Measure", 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("Default").unwrap().axis_of("Region"), Some(Axis::Row));
assert_eq!(m.views.get("Pivot").unwrap().axis_of("Time"), 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"), Axis::Column); assert_eq!(m.views.get("Pivot").unwrap().axis_of("Channel"), Some(Axis::Column));
} }
#[test] #[test]

6
src/ui/app.rs
View File

@ -813,21 +813,21 @@ impl App {
} }
KeyCode::Char('r') => { KeyCode::Char('r') => {
if let Some(name) = cat_names.get(cat_idx) { 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.dirty = true;
} }
self.mode = AppMode::Normal; self.mode = AppMode::Normal;
} }
KeyCode::Char('c') => { KeyCode::Char('c') => {
if let Some(name) = cat_names.get(cat_idx) { 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.dirty = true;
} }
self.mode = AppMode::Normal; self.mode = AppMode::Normal;
} }
KeyCode::Char('p') => { KeyCode::Char('p') => {
if let Some(name) = cat_names.get(cat_idx) { 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.dirty = true;
} }
self.mode = AppMode::Normal; self.mode = AppMode::Normal;

16
src/ui/category_panel.rs
View File

@ -67,16 +67,16 @@ impl<'a> Widget for CategoryPanel<'a> {
let axis = view.axis_of(cat_name); let axis = view.axis_of(cat_name);
let axis_str = match axis { let axis_str = match axis {
Axis::Row => "Row ↕", Some(Axis::Row) => "Row ↕",
Axis::Column => "Col ↔", Some(Axis::Column) => "Col ↔",
Axis::Page => "Page ☰", Some(Axis::Page) => "Page ☰",
Axis::Unassigned => "none", None => "none",
}; };
let axis_color = match axis { let axis_color = match axis {
Axis::Row => Color::Green, Some(Axis::Row) => Color::Green,
Axis::Column => Color::Blue, Some(Axis::Column) => Color::Blue,
Axis::Page => Color::Magenta, Some(Axis::Page) => Color::Magenta,
Axis::Unassigned => Color::DarkGray, None => Color::DarkGray,
}; };
let item_count = self.model.category(cat_name).map(|c| c.items.len()).unwrap_or(0); let item_count = self.model.category(cat_name).map(|c| c.items.len()).unwrap_or(0);

16
src/ui/tile_bar.rs
View File

@ -41,10 +41,10 @@ impl<'a> Widget for TileBar<'a> {
for (i, cat_name) in cat_names.iter().enumerate() { for (i, cat_name) in cat_names.iter().enumerate() {
let axis = view.axis_of(cat_name); let axis = view.axis_of(cat_name);
let axis_symbol = match axis { let axis_symbol = match axis {
Axis::Row => "↕", Some(Axis::Row) => "↕",
Axis::Column => "↔", Some(Axis::Column) => "↔",
Axis::Page => "☰", Some(Axis::Page) => "☰",
Axis::Unassigned => "─", None => "─",
}; };
let label = format!(" [{cat_name} {axis_symbol}] "); 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) Style::default().fg(Color::Black).bg(Color::Cyan).add_modifier(Modifier::BOLD)
} else { } else {
match axis { match axis {
Axis::Row => Style::default().fg(Color::Green), Some(Axis::Row) => Style::default().fg(Color::Green),
Axis::Column => Style::default().fg(Color::Blue), Some(Axis::Column) => Style::default().fg(Color::Blue),
Axis::Page => Style::default().fg(Color::Magenta), Some(Axis::Page) => Style::default().fg(Color::Magenta),
Axis::Unassigned => Style::default().fg(Color::DarkGray), None => Style::default().fg(Color::DarkGray),
} }
}; };

8
src/view/axis.rs
View File

@ -1,21 +1,19 @@
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum Axis { pub enum Axis {
Row, Row,
Column, Column,
Page, Page,
/// Not yet assigned
Unassigned,
} }
impl std::fmt::Display for Axis { impl std::fmt::Display for Axis {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self { match self {
Axis::Row => write!(f, "Row ↕"), Axis::Row => write!(f, "Row ↕"),
Axis::Column => write!(f, "Col ↔"), Axis::Column => write!(f, "Col ↔"),
Axis::Page => write!(f, "Page ☰"), Axis::Page => write!(f, "Page ☰"),
Axis::Unassigned => write!(f, "─"),
} }
} }
} }

45
src/view/view.rs
View File

@ -61,8 +61,8 @@ impl View {
} }
} }
pub fn axis_of(&self, cat_name: &str) -> Axis { pub fn axis_of(&self, cat_name: &str) -> Option<Axis> {
self.category_axes.get(cat_name).copied().unwrap_or(Axis::Unassigned) self.category_axes.get(cat_name).copied()
} }
pub fn categories_on(&self, axis: Axis) -> Vec<&str> { pub fn categories_on(&self, axis: Axis) -> Vec<&str> {
@ -72,6 +72,7 @@ impl View {
.collect() .collect()
} }
pub fn set_page_selection(&mut self, cat_name: &str, item: &str) { pub fn set_page_selection(&mut self, cat_name: &str, item: &str) {
self.page_selections.insert(cat_name.to_string(), item.to_string()); 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 /// Cycle axis for a category: Row → Column → Page → Row
pub fn cycle_axis(&mut self, cat_name: &str) { pub fn cycle_axis(&mut self, cat_name: &str) {
let current = self.axis_of(cat_name); let next = match self.axis_of(cat_name) {
let next = match current { Some(Axis::Row) | None => Axis::Column,
Axis::Row => Axis::Column, Some(Axis::Column) => Axis::Page,
Axis::Column => Axis::Page, Some(Axis::Page) => Axis::Row,
Axis::Page => Axis::Row,
Axis::Unassigned => Axis::Row,
}; };
self.set_axis(cat_name, next); self.set_axis(cat_name, next);
self.selected = (0, 0); self.selected = (0, 0);
@ -140,27 +139,27 @@ mod tests {
#[test] #[test]
fn first_category_assigned_to_row() { fn first_category_assigned_to_row() {
let v = view_with_cats(&["Region"]); let v = view_with_cats(&["Region"]);
assert_eq!(v.axis_of("Region"), Axis::Row); assert_eq!(v.axis_of("Region"), Some(Axis::Row));
} }
#[test] #[test]
fn second_category_assigned_to_column() { fn second_category_assigned_to_column() {
let v = view_with_cats(&["Region", "Product"]); 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] #[test]
fn third_and_later_categories_assigned_to_page() { fn third_and_later_categories_assigned_to_page() {
let v = view_with_cats(&["Region", "Product", "Time", "Scenario"]); let v = view_with_cats(&["Region", "Product", "Time", "Scenario"]);
assert_eq!(v.axis_of("Time"), Axis::Page); assert_eq!(v.axis_of("Time"), Some(Axis::Page));
assert_eq!(v.axis_of("Scenario"), Axis::Page); assert_eq!(v.axis_of("Scenario"), Some(Axis::Page));
} }
#[test] #[test]
fn set_axis_changes_assignment() { fn set_axis_changes_assignment() {
let mut v = view_with_cats(&["Region", "Product"]); let mut v = view_with_cats(&["Region", "Product"]);
v.set_axis("Region", Axis::Column); v.set_axis("Region", Axis::Column);
assert_eq!(v.axis_of("Region"), Axis::Column); assert_eq!(v.axis_of("Region"), Some(Axis::Column));
} }
#[test] #[test]
@ -172,9 +171,9 @@ mod tests {
} }
#[test] #[test]
fn axis_of_unknown_category_returns_unassigned() { fn axis_of_unknown_category_returns_none() {
let v = View::new("Test"); let v = View::new("Test");
assert_eq!(v.axis_of("Ghost"), Axis::Unassigned); assert_eq!(v.axis_of("Ghost"), None);
} }
#[test] #[test]
@ -209,7 +208,7 @@ mod tests {
fn cycle_axis_row_to_column() { fn cycle_axis_row_to_column() {
let mut v = view_with_cats(&["Region", "Product"]); let mut v = view_with_cats(&["Region", "Product"]);
v.cycle_axis("Region"); v.cycle_axis("Region");
assert_eq!(v.axis_of("Region"), Axis::Column); assert_eq!(v.axis_of("Region"), Some(Axis::Column));
} }
#[test] #[test]
@ -217,14 +216,14 @@ mod tests {
let mut v = view_with_cats(&["Region", "Product"]); let mut v = view_with_cats(&["Region", "Product"]);
v.set_axis("Product", Axis::Column); v.set_axis("Product", Axis::Column);
v.cycle_axis("Product"); v.cycle_axis("Product");
assert_eq!(v.axis_of("Product"), Axis::Page); assert_eq!(v.axis_of("Product"), Some(Axis::Page));
} }
#[test] #[test]
fn cycle_axis_page_to_row() { fn cycle_axis_page_to_row() {
let mut v = view_with_cats(&["Region", "Product", "Time"]); let mut v = view_with_cats(&["Region", "Product", "Time"]);
v.cycle_axis("Time"); v.cycle_axis("Time");
assert_eq!(v.axis_of("Time"), Axis::Row); assert_eq!(v.axis_of("Time"), Some(Axis::Row));
} }
#[test] #[test]
@ -260,9 +259,9 @@ mod prop_tests {
for c in &cats { v.on_category_added(c); } for c in &cats { v.on_category_added(c); }
for c in &cats { for c in &cats {
let axis = v.axis_of(c); 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); "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()), prop_assert!(on_axis.contains(&c.as_str()),
"categories_on({:?}) should contain '{}'", axis, c); "categories_on({:?}) should contain '{}'", axis, c);
} }
@ -288,9 +287,9 @@ mod prop_tests {
fn on_category_added_idempotent(cats in unique_cat_names()) { fn on_category_added_idempotent(cats in unique_cat_names()) {
let mut v = View::new("T"); let mut v = View::new("T");
for c in &cats { v.on_category_added(c); } 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<Option<Axis>> = cats.iter().map(|c| v.axis_of(c)).collect();
for c in &cats { v.on_category_added(c); } 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<Option<Axis>> = cats.iter().map(|c| v.axis_of(c)).collect();
prop_assert_eq!(axes_before, axes_after); prop_assert_eq!(axes_before, axes_after);
} }
@ -306,7 +305,7 @@ mod prop_tests {
let idx = target_idx % cats.len(); let idx = target_idx % cats.len();
let cat = &cats[idx]; let cat = &cats[idx];
v.set_axis(cat, axis); 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 /// After set_axis(cat, X), cat is NOT in categories_on(Y) for Y ≠ X