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

Merge branch 'main' into worktree-improvise-ewi-formula-crate

Browse Source 
# Conflicts:
#	src/model/types.rs
#	src/view/layout.rs
This commit is contained in:
Edward Langley
2026-04-15 21:11:55 -07:00
parent 3fbf56ec8b 6d4b19a940
commit f1229a60e4
16 changed files with 1170 additions and 497 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
.gitignore vendored
View File

@ -23,3 +23,4 @@ bench/*.txt
*.swp
proptest-regressions/
.cursor
.claude/worktrees/

4
Cargo.lock generated
View File

@ -756,6 +756,10 @@ name = "improvise-formula"
version = "0.1.0-rc2"
dependencies = [
"anyhow",
"pest",
"pest_derive",
"pest_meta",
"proptest",
"serde",
]

6
context/design-principles.md
View File

@ -32,6 +32,12 @@ Commands compose via `Binding::Sequence` — a keymap entry can chain multiple
commands, each contributing effects independently. The `o` key (add row + begin
editing) is two commands composed at the binding level, not a monolithic handler.
### Decompose rather than early return
Early Returns usually are a signal of mixed responsibilities: if an early return
would clarify a function, consider how the function could be decomposed for the
same effect without the early return.
---
## 2. Polymorphism Over Conditionals

6
crates/improvise-formula/Cargo.toml
View File

@ -8,4 +8,10 @@ repository = "https://github.com/fiddlerwoaroof/improvise"
[dependencies]
anyhow = "1"
pest = "2.8.6"
pest_derive = "2.8.6"
serde = { version = "1", features = ["derive"] }
[dev-dependencies]
pest_meta = "2.8.6"
proptest = "1"

91
crates/improvise-formula/src/formula.pest Normal file
View File

@ -0,0 +1,91 @@
// Formula grammar for improvise.
//
// A formula has the form: TARGET = EXPR [WHERE filter]
// See parser.rs for the tree walker that produces a Formula AST.
//
// Identifier rules (bare_ident / pipe_quoted) mirror `bare_name` and
// `pipe_quoted` in src/persistence/improv.pest: bare identifiers are
// alphanumeric plus `_` and `-`, with no internal spaces; multi-word
// names must be pipe-quoted.
// Auto-skip horizontal whitespace between tokens in non-atomic rules.
WHITESPACE = _{ " " | "\t" }
// ---- top-level ----------------------------------------------------------
formula = { SOI ~ target ~ "=" ~ expr ~ where_clause? ~ EOI }
// The target keeps its raw text (including pipes, if any) — we capture
// the span directly rather than walking into its children.
target = { identifier }
where_clause = { ^"WHERE" ~ identifier ~ "=" ~ filter_value }
// ---- expressions --------------------------------------------------------
// Used by parse_expr() — forces a standalone expression to consume the
// whole input, so `1 + 2 3` fails instead of silently dropping " 3".
expr_eoi = { SOI ~ expr ~ EOI }
expr = { add_expr }
add_expr = { mul_expr ~ (add_op ~ mul_expr)* }
add_op = { "+" | "-" }
mul_expr = { pow_expr ~ (mul_op ~ pow_expr)* }
mul_op = { "*" | "/" }
pow_expr = { unary ~ (pow_op ~ unary)? }
pow_op = { "^" }
unary = { unary_minus | primary }
unary_minus = { "-" ~ primary }
primary = {
number
| agg_call
| if_expr
| paren_expr
| ref_expr
}
paren_expr = { "(" ~ expr ~ ")" }
// Aggregates with optional inline WHERE filter inside the parens.
agg_call = { agg_func ~ "(" ~ expr ~ inline_where? ~ ")" }
agg_func = { ^"SUM" | ^"AVG" | ^"MIN" | ^"MAX" | ^"COUNT" }
inline_where = { ^"WHERE" ~ identifier ~ "=" ~ filter_value }
// IF(cond, then, else). Comparison is a standalone rule because comparison
// operators are not valid in general expressions — only inside an IF condition.
if_expr = { ^"IF" ~ "(" ~ comparison ~ "," ~ expr ~ "," ~ expr ~ ")" }
comparison = { expr ~ cmp_op ~ expr }
cmp_op = { "!=" | "<=" | ">=" | "<" | ">" | "=" }
// A reference to an item. `SUM` and `IF` without parens fall through to
// this rule because agg_call / if_expr require a "(" and otherwise fail.
ref_expr = { identifier }
// ---- identifiers --------------------------------------------------------
//
// Mirror of improv.pest's bare_name / pipe_quoted.
identifier = ${ pipe_quoted | bare_ident }
// Backslash escapes inside pipes: \| literal pipe, \\ backslash, \n newline.
pipe_quoted = @{ "|" ~ ("\\" ~ ANY | !"|" ~ ANY)* ~ "|" }
bare_ident = @{
(ASCII_ALPHA | "_") ~ (ASCII_ALPHANUMERIC | "_" | "-")*
}
// ---- literal values -----------------------------------------------------
filter_value = { string | pipe_quoted | bare_ident }
string = @{ "\"" ~ (!"\"" ~ ANY)* ~ "\"" }
number = @{
ASCII_DIGIT+ ~ ("." ~ ASCII_DIGIT*)?
| "." ~ ASCII_DIGIT+
}

1198
crates/improvise-formula/src/parser.rs
View File

File diff suppressed because it is too large Load Diff

86
src/command/cmd/commit.rs
View File

@ -137,27 +137,89 @@ mod tests {
// ── Commit commands (mode-specific buffer consumers) ────────────────────────
/// Commit a cell value: for synthetic records keys, stage in drill pending edits
/// or apply directly; for real keys, write to the model.
fn commit_cell_value(key: &CellKey, value: &str, effects: &mut Vec<Box<dyn Effect>>) {
if let Some((record_idx, col_name)) = crate::view::synthetic_record_info(key) {
effects.push(Box::new(effect::SetDrillPendingEdit {
record_idx,
col_name,
new_value: value.to_string(),
}));
} else if value.is_empty() {
/// in drill mode, or apply directly in plain records mode; for real keys, write
/// to the model.
fn commit_regular_cell_value(key: &CellKey, value: &str, effects: &mut Vec<Box<dyn Effect>>) {
if value.is_empty() {
effects.push(Box::new(effect::ClearCell(key.clone())));
effects.push(effect::mark_dirty());
} else if let Ok(n) = value.parse::<f64>() {
effects.push(Box::new(effect::SetCell(key.clone(), CellValue::Number(n))));
effects.push(effect::mark_dirty());
} else {
effects.push(Box::new(effect::SetCell(
key.clone(),
CellValue::Text(value.to_string()),
)));
}
effects.push(effect::mark_dirty());
}
/// Stage a synthetic edit in drill state so it can be applied atomically on exit.
fn stage_drill_edit(record_idx: usize, col_name: String, value: &str) -> Box<dyn Effect> {
Box::new(effect::SetDrillPendingEdit {
record_idx,
col_name,
new_value: value.to_string(),
})
}
/// Apply a synthetic records-mode edit directly to the underlying model cell.
fn commit_plain_records_edit(
ctx: &CmdContext,
record_idx: usize,
col_name: &str,
value: &str,
effects: &mut Vec<Box<dyn Effect>>,
) {
let Some((orig_key, _)) = ctx
.layout
.records
.as_ref()
.and_then(|records| records.get(record_idx))
else {
return;
};
if col_name == "Value" {
commit_regular_cell_value(orig_key, value, effects);
return;
}
if value.is_empty() {
effects.push(effect::set_status(effect::RECORD_COORDS_CANNOT_BE_EMPTY));
return;
}
let Some(existing_value) = ctx.model.get_cell(orig_key).cloned() else {
return;
};
effects.push(Box::new(effect::ClearCell(orig_key.clone())));
effects.push(Box::new(effect::AddItem {
category: col_name.to_string(),
item: value.to_string(),
}));
effects.push(Box::new(effect::SetCell(
orig_key.clone().with(col_name, value),
existing_value,
)));
effects.push(effect::mark_dirty());
}
fn commit_cell_value(
ctx: &CmdContext,
key: &CellKey,
value: &str,
effects: &mut Vec<Box<dyn Effect>>,
) {
if let Some((record_idx, col_name)) = crate::view::synthetic_record_info(key) {
if ctx.has_drill_state {
effects.push(stage_drill_edit(record_idx, col_name, value));
return;
}
commit_plain_records_edit(ctx, record_idx, &col_name, value, effects);
return;
}
commit_regular_cell_value(key, value, effects);
}
/// Direction to advance after committing a cell edit.
@ -187,7 +249,7 @@ impl Cmd for CommitAndAdvance {
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
commit_cell_value(&self.key, &self.value, &mut effects);
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
match self.advance {
AdvanceDir::Down => {
let adv = EnterAdvance {

15
src/command/cmd/core.rs
View File

@ -43,6 +43,8 @@ pub struct CmdContext<'a> {
/// View navigation stacks (for drill back/forward)
pub view_back_stack: &'a [String],
pub view_forward_stack: &'a [String],
/// Whether the app currently has an active drill snapshot.
pub has_drill_state: bool,
/// Display value at the cursor — works uniformly for pivot and records mode.
pub display_value: String,
/// How many data rows/cols fit on screen (for viewport scrolling).
@ -55,9 +57,22 @@ pub struct CmdContext<'a> {
}
impl<'a> CmdContext<'a> {
/// Return true when the current layout is a records-mode layout.
pub fn is_records_mode(&self) -> bool {
self.layout.is_records_mode()
}
pub fn cell_key(&self) -> Option<CellKey> {
self.layout.cell_key(self.selected.0, self.selected.1)
}
/// Return synthetic record coordinates for the current cursor, if any.
pub fn synthetic_record_at_cursor(&self) -> Option<(usize, String)> {
self.cell_key()
.as_ref()
.and_then(crate::view::synthetic_record_info)
}
pub fn row_count(&self) -> usize {
self.layout.row_count()
}

7
src/command/cmd/grid.rs
View File

@ -443,12 +443,7 @@ impl Cmd for AddRecordRow {
"add-record-row"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let is_records = ctx
.cell_key()
.as_ref()
.and_then(crate::view::synthetic_record_info)
.is_some();
if !is_records {
if !ctx.is_records_mode() {
return vec![effect::set_status(
"add-record-row only works in records mode",
)];

1
src/command/cmd/mod.rs
View File

@ -71,6 +71,7 @@ pub(super) mod test_helpers {
buffers: &EMPTY_BUFFERS,
view_back_stack: &[],
view_forward_stack: &[],
has_drill_state: false,
display_value: {
let key = layout.cell_key(sr, sc);
key.as_ref()

6
src/command/cmd/mode.rs
View File

@ -228,11 +228,7 @@ impl Cmd for EditOrDrill {
.unwrap_or(false)
});
// In records mode (synthetic key), always edit directly — no drilling.
let is_synthetic = ctx
.cell_key()
.as_ref()
.and_then(crate::view::synthetic_record_info)
.is_some();
let is_synthetic = ctx.synthetic_record_at_cursor().is_some();
let is_aggregated = !is_synthetic && regular_none;
if is_aggregated {
let Some(key) = ctx.cell_key().clone() else {

2
src/model/types.rs
View File

@ -1831,7 +1831,7 @@ mod five_category {
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::None);
assert_eq!(v.axis_of("_Measure"), Axis::Page);
}
#[test]

162
src/ui/app.rs
View File

@ -280,6 +280,7 @@ impl App {
tile_cat_idx: self.tile_cat_idx,
view_back_stack: &self.view_back_stack,
view_forward_stack: &self.view_forward_stack,
has_drill_state: self.drill_state.is_some(),
display_value: {
let key = layout.cell_key(sel_row, sel_col);
if let Some(k) = &key {
@ -708,6 +709,167 @@ mod tests {
);
}
/// Regression: pressing `o` in an empty records view should create the
/// first synthetic row instead of only entering edit mode on empty space.
#[test]
fn add_record_row_in_empty_records_view_creates_first_row() {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.model.category_mut("Region").unwrap().add_item("East");
let mut app = App::new(wb, None);
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
.unwrap();
assert!(app.layout.is_records_mode(), "R should enter records mode");
assert_eq!(app.layout.row_count(), 0, "fresh records view starts empty");
app.handle_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::NONE))
.unwrap();
assert_eq!(
app.layout.row_count(),
1,
"o should create the first record row in an empty records view"
);
assert!(
matches!(app.mode, AppMode::Editing { .. }),
"o should leave the app in edit mode, got {:?}",
app.mode
);
}
/// Regression: editing the first row in a blank model's records view
/// should persist the typed value even though plain records mode does not
/// use drill state. With _Measure as the first column, `o` lands on it;
/// type a measure name, Tab to Value, type the number, Enter to commit.
#[test]
fn edit_record_row_in_blank_model_persists_value() {
use crate::model::cell::CellKey;
let mut app = App::new(Workbook::new("T"), None);
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
.unwrap();
// `o` adds a record row and enters edit at (0, 0) = _Measure column
app.handle_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::NONE))
.unwrap();
// Type a measure name
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Char('e'), KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Char('v'), KeyModifiers::NONE))
.unwrap();
// Tab to commit _Measure and move to Value column
app.handle_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE))
.unwrap();
// Type the value
app.handle_key(KeyEvent::new(KeyCode::Char('5'), KeyModifiers::NONE))
.unwrap();
// Enter to commit
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
assert_eq!(
app.workbook.model.get_cell(&CellKey::new(vec![
("_Measure".to_string(), "Rev".to_string()),
])),
Some(&crate::model::cell::CellValue::Number(5.0)),
"editing a synthetic row in plain records mode should write the value"
);
}
/// Drill-view edits should stay staged in drill state until the user
/// navigates back, at which point ApplyAndClearDrill writes them through.
#[test]
fn drill_edit_is_staged_until_view_back() {
use crate::model::cell::{CellKey, CellValue};
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_category("Month").unwrap();
wb.model.category_mut("Region").unwrap().add_item("East");
wb.model.category_mut("Month").unwrap().add_item("Jan");
let record_key = CellKey::new(vec![
("Month".to_string(), "Jan".to_string()),
("Region".to_string(), "East".to_string()),
]);
wb.model.set_cell(record_key.clone(), CellValue::Number(1.0));
let mut app = App::new(wb, None);
app.handle_key(KeyEvent::new(KeyCode::Char('>'), KeyModifiers::NONE))
.unwrap();
assert!(app.drill_state.is_some(), "drill should create drill state");
let value_col = (0..app.layout.col_count())
.find(|&col| app.layout.col_label(col) == "Value")
.expect("drill view should include a Value column");
app.workbook.active_view_mut().selected = (0, value_col);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Char('9'), KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
assert_eq!(
app.workbook.model.get_cell(&record_key),
Some(&CellValue::Number(1.0)),
"drill edit should remain staged until leaving the drill view"
);
assert_eq!(
app.drill_state
.as_ref()
.and_then(|s| s.pending_edits.get(&(0, "Value".to_string()))),
Some(&"9".to_string()),
"drill edit should be recorded in pending_edits"
);
app.handle_key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Char('<'), KeyModifiers::NONE))
.unwrap();
assert_eq!(
app.workbook.model.get_cell(&record_key),
Some(&CellValue::Number(9.0)),
"leaving drill view should apply the staged edit"
);
}
/// Suspected bug: blanking a records-mode category coordinate should not
/// create an item with an empty name.
#[test]
fn blanking_records_category_does_not_create_empty_item() {
use crate::model::cell::{CellKey, CellValue};
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.model.category_mut("Region").unwrap().add_item("East");
wb.model.set_cell(
CellKey::new(vec![("Region".to_string(), "East".to_string())]),
CellValue::Number(1.0),
);
let mut app = App::new(wb, None);
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
for _ in 0..4 {
app.handle_key(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE))
.unwrap();
}
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
assert!(
!app.workbook.model.category("Region").unwrap().items.contains_key(""),
"records-mode edits should not create empty category items"
);
}
#[test]
fn command_mode_buffer_cleared_on_reentry() {
use crossterm::event::KeyEvent;

6
src/ui/effect.rs
View File

@ -6,6 +6,8 @@ use crate::view::Axis;
use super::app::{App, AppMode};
pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty";
/// A discrete state change produced by a command.
/// Effects know how to apply themselves to the App.
pub trait Effect: Debug {
@ -459,6 +461,10 @@ impl Effect for ApplyAndClearDrill {
};
app.workbook.model.set_cell(orig_key.clone(), value);
} else {
if new_value.is_empty() {
app.status_msg = RECORD_COORDS_CANNOT_BE_EMPTY.to_string();
continue;
}
// Rename a coordinate: remove old cell, insert new with updated coord
let value = match app.workbook.model.get_cell(orig_key) {
Some(v) => v.clone(),

69
src/view/layout.rs
View File

@ -1,6 +1,7 @@
use std::rc::Rc;
use crate::model::Model;
use crate::model::category::CategoryKind;
use crate::model::cell::{CellKey, CellValue};
use crate::view::{Axis, View};
@ -92,14 +93,13 @@ impl GridLayout {
let page_coords = page_cats
.iter()
.map(|cat| {
.filter_map(|cat| {
let items: Vec<String> = model.effective_item_names(cat);
let sel = view
.page_selection(cat)
.map(String::from)
.or_else(|| items.first().cloned())
.unwrap_or_default();
(cat.clone(), sel)
.or_else(|| items.first().cloned())?;
Some((cat.clone(), sel))
})
.collect();
@ -132,22 +132,12 @@ impl GridLayout {
page_coords: Vec<(String, String)>,
none_cats: Vec<String>,
) -> Self {
// Filter cells by page_coords
let partial: Vec<(String, String)> = page_coords.clone();
let mut records: Vec<(CellKey, CellValue)> = if partial.is_empty() {
model
let mut records: Vec<(CellKey, CellValue)> = model
.data
.iter_cells()
.map(|(k, v)| (k, v.clone()))
.collect()
} else {
model
.data
.matching_cells(&partial)
.matching_cells(&page_coords)
.into_iter()
.map(|(k, v)| (k, v.clone()))
.collect()
};
.collect();
// Sort for deterministic ordering
records.sort_by(|a, b| a.0.0.cmp(&b.0.0));
@ -157,12 +147,22 @@ impl GridLayout {
.collect();
// Synthesize col items: one per non-virtual category + "Value"
let cat_names: Vec<String> = model
let mut cat_names: Vec<String> = model
.category_names()
.into_iter()
.filter(|c| !c.starts_with('_'))
.filter(|c| {
let kind = model.category(c).map(|cat| cat.kind);
!matches!(kind, Some(CategoryKind::VirtualIndex | CategoryKind::VirtualDim))
})
.map(String::from)
.collect();
// _Measure goes last among category columns (right before Value)
cat_names.sort_by_key(|c| {
matches!(
model.category(c).map(|cat| cat.kind),
Some(CategoryKind::VirtualMeasure)
)
});
let mut col_items: Vec<AxisEntry> = cat_names
.iter()
.map(|c| AxisEntry::DataItem(vec![c.clone()]))
@ -596,6 +596,8 @@ mod tests {
]),
CellValue::Number(50.0),
);
// Records mode setup: _Measure should not filter
wb.active_view_mut().set_axis("_Measure", Axis::None);
wb
}
@ -701,6 +703,35 @@ mod tests {
assert_eq!(dim, "Region");
}
/// Regression test for improvise-rbv: records mode should include _Measure
/// as a _Dim column so the measure name is visible per-record, and "Value"
/// must remain the last column.
#[test]
fn records_mode_includes_measure_in_dim_columns() {
let mut wb = records_workbook();
let v = wb.active_view_mut();
v.set_axis("_Index", Axis::Row);
v.set_axis("_Dim", Axis::Column);
let layout = GridLayout::new(&wb.model, wb.active_view());
assert!(layout.is_records_mode());
let cols: Vec<String> = (0..layout.col_count())
.map(|i| layout.col_label(i))
.collect();
assert!(
cols.contains(&"_Measure".to_string()),
"records mode should include _Measure column; got {:?}",
cols
);
assert!(cols.contains(&"Region".to_string()));
assert!(cols.contains(&"Value".to_string()));
assert_eq!(
cols.last().unwrap(),
"Value",
"Value must be the last column so cursor defaults land on it; got {:?}",
cols
);
}
fn coord(pairs: &[(&str, &str)]) -> CellKey {
CellKey::new(
pairs

5
src/workbook.rs
View File

@ -26,7 +26,9 @@ pub struct Workbook {
impl Workbook {
/// Create a new workbook with a fresh `Model` and a single `Default` view.
/// Virtual categories (`_Index`, `_Dim`, `_Measure`) are registered on the
/// default view with their conventional axes (`_Index`=Row, `_Dim`=Column).
/// default view with their conventional axes (`_Index`=Row, `_Dim`=Column,
/// `_Measure`=Page so aggregated pivot views show a single measure by
/// default; see improvise-kos).
pub fn new(name: impl Into<String>) -> Self {
let model = Model::new(name);
let mut views = IndexMap::new();
@ -42,6 +44,7 @@ impl Workbook {
}
view.set_axis("_Index", Axis::Row);
view.set_axis("_Dim", Axis::Column);
view.set_axis("_Measure", Axis::Page);
}
wb
}