Merge pull request #129 from fiddlerwoaroof/fix/label-and-views
CI / Build all bundles (push) Has been cancelled
CI / Test and lint (host) (push) Has been cancelled
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
CI / Browser JS unit tests (node --test) (push) Has been cancelled
CI / Knowledge base consistency (push) Has been cancelled
CI / Lint the harness crate (push) Has been cancelled
CI / Browser harness (OPFS contract + shell) (push) Has been cancelled
CI / Deploy to GitHub Pages (push) Has been cancelled
CI / Build all bundles (push) Has been cancelled
CI / Test and lint (host) (push) Has been cancelled
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
CI / Browser JS unit tests (node --test) (push) Has been cancelled
CI / Knowledge base consistency (push) Has been cancelled
CI / Lint the harness crate (push) Has been cancelled
CI / Browser harness (OPFS contract + shell) (push) Has been cancelled
CI / Deploy to GitHub Pages (push) Has been cancelled
fix: a Label category survives a save, and a new view does not pivot it
This commit is contained in:
@@ -463,6 +463,36 @@ auto Row/Column assignment.
|
||||
`Category::add_item` deduplicates by name and assigns IDs from a private
|
||||
counter, so external code cannot fabricate an `ItemId`.
|
||||
|
||||
### Seating takes the kind, because a name cannot carry it (improvise-cj4u)
|
||||
|
||||
`View::on_category_added` takes a `CategoryKind` beside the name. It used to
|
||||
take the name alone and ask `of_name`, which reads `Label` as `Regular` by
|
||||
design — so the *default* view got a label on `Axis::None` (because
|
||||
`Workbook::add_label_category` set it explicitly afterwards) while
|
||||
`Workbook::create_view`, which only ever had names to hand, seated the same
|
||||
label on Row. A high-cardinality description column therefore became a pivot
|
||||
axis in every view created after it, and on an already-pivoted document it
|
||||
landed on `Page`, filtering the whole document down to one description.
|
||||
|
||||
Two things follow, and the second is the one worth carrying.
|
||||
|
||||
**The parameter is the fix, not a `set_axis` after the fact.** The old
|
||||
`add_label_category` seated the category and then corrected it, which is two
|
||||
answers to "where does this category go" — and only one of the two callers of
|
||||
`on_category_added` knew to write the second. The correction is now deleted:
|
||||
seating is told the kind and answers once. `add_label_category_sets_none_axis_
|
||||
on_all_views` and `label_category_defaults_to_none_axis` both still pass, and
|
||||
both fail if the parameter is ignored, so the consolidation kept its evidence.
|
||||
|
||||
**`of_name` survives, narrowed.** Every remaining caller either holds no
|
||||
`Category` (the view's own `is_records_layout`, `holds_a_regular`) or is test
|
||||
code building a view from bare names. That residue is real: `holds_a_regular`
|
||||
asks of the categories *already seated* on an axis whether one of them pivots,
|
||||
and it asks by name, so a label a user deliberately moved onto Row still counts
|
||||
as occupying it. That is the right answer — a category the user put on Row is
|
||||
pivoting there — but it is an answer reached by a different route from the one
|
||||
above, and a future `View` that stores kinds should collapse the two.
|
||||
|
||||
### Removing a name removes the formulas targeting it (improvise-g3no)
|
||||
|
||||
`Model::remove_category` has always ended by dropping every formula whose
|
||||
|
||||
@@ -154,6 +154,70 @@ non-atomic rules produce the whitespace-separated tokens pest's implicit
|
||||
with three or more alternatives (nested `Choice`s) pick deeper branches less
|
||||
often — good enough for random generation, not a uniform distribution.
|
||||
|
||||
### The category kind is a line, not a header suffix (improvise-dy3j)
|
||||
|
||||
`CategoryKind::Label` had no spelling in the format at all. `format_md` wrote
|
||||
every category as a bare `## Category: Name` and `build_category` read every
|
||||
section back through `add_category`, so the kind was dropped on the way out
|
||||
**and** on the way in. A round-trip equality test cannot see a loss both sides
|
||||
share, which is why the existing suite was silent — the same shape as the
|
||||
`Formula::raw` traps above, and the reason the new tests assert the *parsed*
|
||||
`Category::kind` rather than the emitted text.
|
||||
|
||||
The consequence was a lost document, not a lost flag. `Label` sits outside
|
||||
`MAX_CATEGORIES = 12` on purpose, and the import wizard's own precondition
|
||||
scores a label as zero, so it will happily build twelve categories plus a
|
||||
label. On load every label came back `Regular`, the thirteenth was refused, and
|
||||
`parse_md` failed: **any document where regular + label exceeded twelve was
|
||||
permanently unloadable.** Below the limit the failure was quieter — a label
|
||||
silently became a pivot category, entering `regular_category_names()` pickers
|
||||
and gaining axis auto-assignment.
|
||||
|
||||
**The ticket proposed `## Category: Note [label]`, and the grammar forbids
|
||||
it.** A section header takes its name as the **whole line**, totally
|
||||
(improvise-pczv) — `header_name` is `pipe_quoted ~ &NEWLINE | line_name` — so a
|
||||
suffix is not a suffix, it is part of a category literally named `Note
|
||||
[label]`. The two readings are indistinguishable, and adopting the suffix would
|
||||
have made a legal name unspellable while quietly re-opening the ambiguity
|
||||
`header_name` exists to close.
|
||||
|
||||
So the kind is an **entry inside the section**, beside the item lines:
|
||||
|
||||
```
|
||||
## Category: Note
|
||||
kind: label
|
||||
- see the record view
|
||||
```
|
||||
|
||||
Three properties earned it. It uses a **real token** (`"kind: "`), not a
|
||||
lookahead, because the grammar-walking generator reads the grammar through
|
||||
`pest_meta` and *skips predicates* — a rule guarded only by a lookahead is a
|
||||
rule the generator can violate, which is why `WHERE` carries `sp+`. It cannot
|
||||
collide with `item_list`, `grouped_item` or `group_hierarchy`, each of which
|
||||
needs a `- ` or `> ` lead. And **absence is the compatible reading**: `kind_kw`
|
||||
spells only `label`, the writer emits the line only for a label, so every file
|
||||
written before this commit parses unchanged and every pivot category's section
|
||||
is byte-identical to what it was. That is also why `parse_kind_kw` is an
|
||||
exhaustive match returning `Result` rather than a `bool` read off the rule's
|
||||
mere presence — a second keyword must fail loudly at the walker, not be read as
|
||||
`label` because it arrived on a `kind_line`.
|
||||
|
||||
One case the format already allowed and this had to answer: two
|
||||
`## Category:` sections naming one category are one category (improvise-349m),
|
||||
merged in pass 1. Absence is not a claim, so the merge keeps a stated kind
|
||||
rather than letting a later kindless section erase it.
|
||||
|
||||
**The generator coverage this section demands was measured, not assumed.**
|
||||
`kind_line` is an alternative of `category_entry`, which the grammar walk
|
||||
reaches generically, so nothing in the generator needed changing — but that is
|
||||
the kind of claim this section exists to say must be checked. Probed with a
|
||||
throwaway test over 500 files from `generator::improv_file()`: **14 carried a
|
||||
`kind: label` line and all 14 parsed to a `CategoryKind::Label`**, so at the
|
||||
default 256 cases each proptest run exercises the new production about seven
|
||||
times. The probe was removed rather than kept: its rate depends on proptest's
|
||||
internal RNG, so as a permanent assertion it would fail on a dependency bump
|
||||
for a reason unrelated to the format.
|
||||
|
||||
### The tree walkers are infallible by construction
|
||||
|
||||
Every `build_*` in `parser.rs` operates on a `Pair` pest has already
|
||||
|
||||
@@ -1143,3 +1143,46 @@ luck. It is the same shape as `analyze_records`' five-iteration guard.
|
||||
This one cannot be made to fail by re-introducing a `HashMap` *reliably*, which
|
||||
is why the test that carries the load is its sibling above; kept because the
|
||||
repetition is what documents that hash order was the defect.
|
||||
|
||||
### a_view_created_after_a_label_still_keeps_it_off_the_axes
|
||||
|
||||
`View::on_category_added` took a name and asked `CategoryKind::of_name`, which
|
||||
reads `Label` as `Regular` by design. `Workbook::add_label_category` papered
|
||||
over that by calling `set_axis(name, Axis::None)` straight afterwards, so the
|
||||
views existing *at that moment* were right; `Workbook::create_view` had only
|
||||
names to hand and had no such correction, so every view created after the label
|
||||
pivoted it. Reproduced at `d46ec59`: `Description` on `Axis::Row` in a view
|
||||
created one line after `add_label_category`.
|
||||
|
||||
The fix gives seating the kind, and the two `set_axis` corrections are deleted —
|
||||
one answer rather than two, of which only one caller knew to write the second.
|
||||
|
||||
### a_view_created_after_a_regular_category_still_seats_it_on_row
|
||||
|
||||
The control, and the direction the bug's own shape cannot supply. Seating
|
||||
everything on `Axis::None` satisfies the label assertion above completely, so
|
||||
without this a blanket `unseated = true` reads as a fix.
|
||||
|
||||
Fault injected: `let unseated = true;`. This test fails (`left: None right:
|
||||
Row`) along with `a_label_does_not_displace_a_regular_category_in_a_new_view`
|
||||
and `first_category_goes_to_row_second_to_column_rest_to_page`, while every
|
||||
label assertion in the repository still passes.
|
||||
|
||||
### a_label_does_not_displace_a_regular_category_in_a_new_view
|
||||
|
||||
The consequence the per-category assertions cannot state, because seating is
|
||||
**order-dependent**: the first regular category takes Row, the second Column,
|
||||
the rest Page. A label wrongly counted as regular therefore does not merely sit
|
||||
in the wrong place — it *consumes the slot* the next real category wanted, and
|
||||
on an already-pivoted document it pushes that category to `Page`, which
|
||||
filters the whole document down to one value.
|
||||
|
||||
The fixture adds the label first and the regular category second, so the
|
||||
displacement is what is measured rather than the label's own axis.
|
||||
|
||||
Fault injected: restoring `let unseated = CategoryKind::of_name(cat_name)
|
||||
.is_virtual() || self.is_records_layout();`. Five tests fail, including the two
|
||||
pre-existing ones (`add_label_category_sets_none_axis_on_all_views`,
|
||||
`label_category_defaults_to_none_axis`) that used to be carried by the deleted
|
||||
`set_axis` correction — which is the evidence that the consolidation did not
|
||||
cost coverage.
|
||||
|
||||
@@ -1288,3 +1288,69 @@ the parse would fail the test for the wrong reason.
|
||||
The absent case, so the field above is something the file carried rather than a
|
||||
constant the walker invents. Without it, a walker hard-coding `Some(".4")`
|
||||
would pass the test above.
|
||||
|
||||
### a_label_category_survives_the_round_trip_as_a_label
|
||||
|
||||
`CategoryKind::Label` had no spelling in `.improv` at all: `format_md` wrote
|
||||
every category as a bare `## Category: Name` and `build_category` read every
|
||||
section back through `Workbook::add_category`. The kind was dropped on the way
|
||||
out *and* on the way in, so the loss was symmetric and no round-trip equality
|
||||
test could see it — the same shape the `Formula::format` entry above records.
|
||||
Reproduced at `d46ec59`: a workbook with `Region` (regular) and `Note` (label)
|
||||
came back with `Note.kind == Regular`.
|
||||
|
||||
The test asserts the **parsed** `Category::kind`, never the emitted line, for
|
||||
that reason. Its second assertion — that `Region` still reads back `Regular` —
|
||||
is the control: a reader that made every category a `Label` satisfies the first
|
||||
assertion and fails this one.
|
||||
|
||||
Faults injected, both against the fixed tree:
|
||||
|
||||
- **Writer drops the line** (`if false` around `w!(out, "kind: label")`). Fails
|
||||
`left: Regular right: Label`, and takes
|
||||
`a_document_at_the_category_limit_plus_a_label_reloads` with it.
|
||||
- **Reader ignores the kind it parsed** (`build_category` calls `add_category`
|
||||
unconditionally, discarding `pc.kind`). Same two failures. This is the
|
||||
discriminating one: the file text is *correct* in this variant, so a test
|
||||
comparing emitted output passes.
|
||||
|
||||
### a_document_at_the_category_limit_plus_a_label_reloads
|
||||
|
||||
The P1 half, and the reason improvise-dy3j was not merely a lost flag. `Label`
|
||||
is outside `MAX_CATEGORIES = 12` by design — `Model::add_label_category` has no
|
||||
limit check, and the import wizard's `proposed_category_count` scores a label
|
||||
as zero — so a document with twelve categories and one label is legally
|
||||
constructible and the wizard will build one from any CSV carrying a free-text
|
||||
column. Every label came back `Regular` on load, the thirteenth category was
|
||||
refused, and `parse_md` returned
|
||||
`Err("Maximum of 12 categories reached")`: **the document was permanently
|
||||
unloadable.** Reproduced at `d46ec59` with exactly that error.
|
||||
|
||||
The second assertion (`regular_category_names().len() == 12`) is what keeps the
|
||||
first honest. A "fix" that simply raised or removed the limit would let the
|
||||
document load while putting a high-cardinality label into the pivot count, and
|
||||
this assertion fails on it.
|
||||
|
||||
### a_category_section_with_no_kind_line_loads_as_regular
|
||||
|
||||
The absent case: every `.improv` file written before this commit carries no
|
||||
`kind:` line, and must still load. Without it, a walker that defaulted
|
||||
`PCategory::kind` to `Label` would pass both tests above.
|
||||
|
||||
The input is hand-written text rather than a round trip, deliberately — a round
|
||||
trip cannot exhibit an absent line, because the writer under test is the thing
|
||||
that would have written it.
|
||||
|
||||
### a_loaded_label_category_is_not_on_a_pivot_axis
|
||||
|
||||
The end-to-end check that the two halves of improvise-dy3j/cj4u agree: a label
|
||||
is off every pivot axis in every view after a save and a load, including a view
|
||||
created after the label.
|
||||
|
||||
Worth stating what it does and does not witness. It is **not** a second witness
|
||||
for the kind: `format_md` writes each view's axes explicitly, so `Note: none`
|
||||
carries the seating across the file whatever the kind does. What it witnesses
|
||||
is the *seating* — with the `improvise-cj4u` fix reverted, `create_view`
|
||||
pivots `Note` onto Column in the source workbook, the file faithfully records
|
||||
`Note: column`, and this test fails. It was written while cj4u was still open
|
||||
and failed then, which is how that was established rather than assumed.
|
||||
|
||||
@@ -72,13 +72,12 @@ impl View {
|
||||
.any(|c| CategoryKind::of_name(c).is_dim())
|
||||
}
|
||||
|
||||
/// Seat a newly added category: a virtual on `None`, a regular one on the
|
||||
/// first slot no regular holds. A virtual is **never** bumped (§7).
|
||||
pub fn on_category_added(&mut self, cat_name: &str) {
|
||||
/// Seat a newly added category: only a `Regular` one takes a pivot slot,
|
||||
/// and the kind is the caller's because a name cannot carry it (§7).
|
||||
pub fn on_category_added(&mut self, cat_name: &str, kind: CategoryKind) {
|
||||
if !self.category_axes.contains_key(cat_name) {
|
||||
let virtual_or_records =
|
||||
CategoryKind::of_name(cat_name).is_virtual() || self.is_records_layout();
|
||||
let axis = if virtual_or_records {
|
||||
let unseated = !kind.is_regular() || self.is_records_layout();
|
||||
let axis = if unseated {
|
||||
Axis::None
|
||||
} else {
|
||||
let holds_a_regular = |axis: Axis| {
|
||||
@@ -258,12 +257,13 @@ impl View {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::View;
|
||||
use crate::model::category::CategoryKind;
|
||||
use crate::view::Axis;
|
||||
|
||||
fn view_with_cats(cats: &[&str]) -> View {
|
||||
let mut v = View::new("Test");
|
||||
for &c in cats {
|
||||
v.on_category_added(c);
|
||||
v.on_category_added(c, CategoryKind::of_name(c));
|
||||
}
|
||||
v
|
||||
}
|
||||
@@ -289,7 +289,7 @@ mod tests {
|
||||
v.set_axis("Region", Axis::None);
|
||||
assert!(v.is_records_layout(), "setup");
|
||||
|
||||
v.on_category_added("Product");
|
||||
v.on_category_added("Product", CategoryKind::of_name("Product"));
|
||||
assert_eq!(v.axis_of("Product"), Axis::None);
|
||||
assert_eq!(v.axis_of("_Index"), Axis::Row, "_Index must not be bumped");
|
||||
assert_eq!(v.axis_of("_Dim"), Axis::Column, "_Dim must not be bumped");
|
||||
@@ -305,7 +305,7 @@ mod tests {
|
||||
assert_eq!(v.axis_of("Region"), Axis::Row, "setup");
|
||||
assert!(!v.is_records_layout(), "setup: a pivot view, not records");
|
||||
|
||||
v.on_category_added("Product");
|
||||
v.on_category_added("Product", CategoryKind::of_name("Product"));
|
||||
assert_eq!(
|
||||
v.axis_of("_Measure"),
|
||||
Axis::Column,
|
||||
@@ -538,10 +538,10 @@ mod tests {
|
||||
#[test]
|
||||
fn on_category_removed_forgets_a_collapsed_group() {
|
||||
let mut v = View::new("V");
|
||||
v.on_category_added("Month");
|
||||
v.on_category_added("Month", CategoryKind::of_name("Month"));
|
||||
v.toggle_group_collapse("Month", "Q1");
|
||||
v.on_category_removed("Month");
|
||||
v.on_category_added("Month");
|
||||
v.on_category_added("Month", CategoryKind::of_name("Month"));
|
||||
assert!(
|
||||
!v.is_group_collapsed("Month", "Q1"),
|
||||
"a re-added category came back with its group collapsed"
|
||||
@@ -553,7 +553,7 @@ mod tests {
|
||||
#[test]
|
||||
fn on_item_removed_forgets_a_hidden_item() {
|
||||
let mut v = View::new("V");
|
||||
v.on_category_added("Region");
|
||||
v.on_category_added("Region", CategoryKind::of_name("Region"));
|
||||
v.hide_item("Region", "East");
|
||||
v.hide_item("Region", "West");
|
||||
v.on_item_removed("Region", "East");
|
||||
@@ -569,7 +569,7 @@ mod tests {
|
||||
#[test]
|
||||
fn on_item_removed_forgets_a_page_selection_naming_it() {
|
||||
let mut v = View::new("V");
|
||||
v.on_category_added("Region");
|
||||
v.on_category_added("Region", CategoryKind::of_name("Region"));
|
||||
v.set_page_selection("Region", "East");
|
||||
v.on_item_removed("Region", "West");
|
||||
assert_eq!(
|
||||
@@ -625,6 +625,7 @@ mod tests {
|
||||
#[cfg(test)]
|
||||
mod prop_tests {
|
||||
use super::View;
|
||||
use crate::model::category::CategoryKind;
|
||||
use crate::view::Axis;
|
||||
use proptest::prelude::*;
|
||||
|
||||
@@ -638,7 +639,7 @@ mod prop_tests {
|
||||
#[test]
|
||||
fn axis_of_and_categories_on_consistent(cats in unique_cat_names()) {
|
||||
let mut v = View::new("T");
|
||||
for c in &cats { v.on_category_added(c); }
|
||||
for c in &cats { v.on_category_added(c, CategoryKind::of_name(c)); }
|
||||
for c in &cats {
|
||||
let axis = v.axis_of(c);
|
||||
let on_axis = v.categories_on(axis);
|
||||
@@ -651,7 +652,7 @@ mod prop_tests {
|
||||
#[test]
|
||||
fn each_category_on_exactly_one_axis(cats in unique_cat_names()) {
|
||||
let mut v = View::new("T");
|
||||
for c in &cats { v.on_category_added(c); }
|
||||
for c in &cats { v.on_category_added(c, CategoryKind::of_name(c)); }
|
||||
let all_axes = [Axis::Row, Axis::Column, Axis::Page, Axis::None];
|
||||
for c in &cats {
|
||||
let count = all_axes.iter()
|
||||
@@ -666,9 +667,9 @@ mod prop_tests {
|
||||
#[test]
|
||||
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); }
|
||||
for c in &cats { v.on_category_added(c, CategoryKind::of_name(c)); }
|
||||
let axes_before: Vec<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, CategoryKind::of_name(c)); }
|
||||
let axes_after: Vec<Axis> = cats.iter().map(|c| v.axis_of(c)).collect();
|
||||
prop_assert_eq!(axes_before, axes_after);
|
||||
}
|
||||
@@ -681,7 +682,7 @@ mod prop_tests {
|
||||
axis in prop_oneof![Just(Axis::Row), Just(Axis::Column), Just(Axis::Page), Just(Axis::None)],
|
||||
) {
|
||||
let mut v = View::new("T");
|
||||
for c in &cats { v.on_category_added(c); }
|
||||
for c in &cats { v.on_category_added(c, CategoryKind::of_name(c)); }
|
||||
let idx = target_idx % cats.len();
|
||||
let cat = &cats[idx];
|
||||
v.set_axis(cat, axis);
|
||||
@@ -696,7 +697,7 @@ mod prop_tests {
|
||||
axis in prop_oneof![Just(Axis::Row), Just(Axis::Column), Just(Axis::Page), Just(Axis::None)],
|
||||
) {
|
||||
let mut v = View::new("T");
|
||||
for c in &cats { v.on_category_added(c); }
|
||||
for c in &cats { v.on_category_added(c, CategoryKind::of_name(c)); }
|
||||
let idx = target_idx % cats.len();
|
||||
let cat = &cats[idx];
|
||||
v.set_axis(cat, axis);
|
||||
@@ -714,7 +715,7 @@ mod prop_tests {
|
||||
#[test]
|
||||
fn total_category_count_consistent(cats in unique_cat_names()) {
|
||||
let mut v = View::new("T");
|
||||
for c in &cats { v.on_category_added(c); }
|
||||
for c in &cats { v.on_category_added(c, CategoryKind::of_name(c)); }
|
||||
let total: usize = [Axis::Row, Axis::Column, Axis::Page]
|
||||
.iter()
|
||||
.map(|&ax| v.categories_on(ax).len())
|
||||
|
||||
@@ -6,7 +6,7 @@ use indexmap::IndexMap;
|
||||
use serde::{Deserialize, Serialize};
|
||||
|
||||
use crate::model::Model;
|
||||
use crate::model::category::{CategoryId, ItemId};
|
||||
use crate::model::category::{CategoryId, CategoryKind, ItemId};
|
||||
use crate::view::{Axis, DEFAULT_VIEW, View};
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
@@ -29,8 +29,8 @@ impl Workbook {
|
||||
active_view: DEFAULT_VIEW.to_string(),
|
||||
};
|
||||
for view in wb.views.values_mut() {
|
||||
for cat_name in wb.model.categories.keys() {
|
||||
view.on_category_added(cat_name);
|
||||
for cat in wb.model.categories.values() {
|
||||
view.on_category_added(&cat.name, cat.kind);
|
||||
}
|
||||
view.set_axis("_Measure", Axis::Page);
|
||||
}
|
||||
@@ -44,7 +44,7 @@ impl Workbook {
|
||||
let name = name.into();
|
||||
let id = self.model.add_category(&name)?;
|
||||
for view in self.views.values_mut() {
|
||||
view.on_category_added(&name);
|
||||
view.on_category_added(&name, CategoryKind::Regular);
|
||||
}
|
||||
Ok(id)
|
||||
}
|
||||
@@ -55,8 +55,7 @@ impl Workbook {
|
||||
let name = name.into();
|
||||
let id = self.model.add_label_category(&name)?;
|
||||
for view in self.views.values_mut() {
|
||||
view.on_category_added(&name);
|
||||
view.set_axis(&name, Axis::None);
|
||||
view.on_category_added(&name, CategoryKind::Label);
|
||||
}
|
||||
Ok(id)
|
||||
}
|
||||
@@ -123,8 +122,8 @@ impl Workbook {
|
||||
pub fn create_view(&mut self, name: impl Into<String>) -> &mut View {
|
||||
let name = name.into();
|
||||
let mut view = View::new(name.clone());
|
||||
for cat_name in self.model.categories.keys() {
|
||||
view.on_category_added(cat_name);
|
||||
for cat in self.model.categories.values() {
|
||||
view.on_category_added(&cat.name, cat.kind);
|
||||
}
|
||||
self.views.insert(name.clone(), view);
|
||||
self.views.get_mut(&name).unwrap()
|
||||
@@ -187,6 +186,42 @@ mod tests {
|
||||
assert_eq!(wb.views.get("Other").unwrap().axis_of("Note"), Axis::None);
|
||||
}
|
||||
|
||||
/// A view created *after* the label must seat it off the pivot axes too.
|
||||
/// Evidence: `context/test-evidence/core.md`.
|
||||
#[test]
|
||||
fn a_view_created_after_a_label_still_keeps_it_off_the_axes() {
|
||||
let mut wb = Workbook::new("Test");
|
||||
wb.add_label_category("Description").unwrap();
|
||||
assert_eq!(wb.active_view().axis_of("Description"), Axis::None);
|
||||
assert_eq!(wb.create_view("Other").axis_of("Description"), Axis::None);
|
||||
}
|
||||
|
||||
/// The control for the test above: a regular category added the same way
|
||||
/// still takes the first free pivot slot, so `None` is not the blanket answer.
|
||||
#[test]
|
||||
fn a_view_created_after_a_regular_category_still_seats_it_on_row() {
|
||||
let mut wb = Workbook::new("Test");
|
||||
wb.add_category("Region").unwrap();
|
||||
assert_eq!(wb.create_view("Other").axis_of("Region"), Axis::Row);
|
||||
}
|
||||
|
||||
/// A label must not consume the Row slot a later regular category wants.
|
||||
/// Evidence: `context/test-evidence/core.md`.
|
||||
#[test]
|
||||
fn a_label_does_not_displace_a_regular_category_in_a_new_view() {
|
||||
let mut wb = Workbook::new("Test");
|
||||
wb.add_label_category("Description").unwrap();
|
||||
wb.add_category("Region").unwrap();
|
||||
|
||||
let v = wb.create_view("Other");
|
||||
assert_eq!(v.axis_of("Description"), Axis::None);
|
||||
assert_eq!(
|
||||
v.axis_of("Region"),
|
||||
Axis::Row,
|
||||
"Row was free — the label must not have taken it"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remove_category_removes_from_all_views() {
|
||||
let mut wb = Workbook::new("Test");
|
||||
|
||||
@@ -257,7 +257,7 @@ mod tests {
|
||||
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
for cat in [QUARTER, NOTE] {
|
||||
view.set_axis(cat, Axis::None);
|
||||
|
||||
@@ -502,7 +502,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis(REGION, Axis::Row);
|
||||
// `Workbook::new` — the constructor a real document comes from — leaves
|
||||
@@ -714,7 +714,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis("Year", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
@@ -826,7 +826,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis("Wide", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
@@ -872,7 +872,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis("Year", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
@@ -962,7 +962,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis(REGION, Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
@@ -1050,7 +1050,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis("Year", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
@@ -1122,7 +1122,7 @@ mod tests {
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
view.on_category_added(cat, CategoryKind::of_name(cat));
|
||||
}
|
||||
view.set_axis("Dup", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
|
||||
@@ -181,6 +181,11 @@ pub fn format_md(workbook: &Workbook) -> String {
|
||||
continue;
|
||||
}
|
||||
w!(out, "\n## Category: {}", quote_header(&cat.name));
|
||||
// The kind is written only where it is not the default, so a pivot
|
||||
// category's section is byte-identical to before (improvise-dy3j).
|
||||
if cat.kind == CategoryKind::Label {
|
||||
w!(out, "kind: label");
|
||||
}
|
||||
// Items are written in insertion order, because that is the order the
|
||||
// grid's axes consume (gotcha 18). A run of bare items shares a line.
|
||||
let mut bare: Vec<String> = Vec::new();
|
||||
|
||||
@@ -36,7 +36,12 @@ category_section = {
|
||||
category_entry*
|
||||
}
|
||||
|
||||
category_entry = _{ group_hierarchy | grouped_item | item_list }
|
||||
category_entry = _{ kind_line | group_hierarchy | grouped_item | item_list }
|
||||
|
||||
// The category's kind. Absent means `regular` — the compatible reading of
|
||||
// every file written before this line existed. context/code/formula-and-io.md.
|
||||
kind_line = { "kind: " ~ kind_kw ~ NEWLINE ~ blank_lines }
|
||||
kind_kw = @{ "label" }
|
||||
|
||||
// Comma-separated bare items (no group): `- Food, Gas, Total`
|
||||
item_list = {
|
||||
|
||||
@@ -7,7 +7,7 @@ use pest::iterators::{Pair, Pairs};
|
||||
use pest_derive::Parser;
|
||||
|
||||
use crate::formula::{AggFunc, parse_formula};
|
||||
use crate::model::category::Group;
|
||||
use crate::model::category::{CategoryKind, Group};
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::view::Axis;
|
||||
use crate::workbook::Workbook;
|
||||
@@ -27,6 +27,9 @@ struct PCategory {
|
||||
/// is load-bearing, so pass 2 must replay this list as written.
|
||||
items: Vec<(String, Option<String>)>,
|
||||
group_parents: Vec<(String, String)>,
|
||||
/// What a `kind:` line said, `Regular` where none did. Absence is not a
|
||||
/// claim, so merging two sections keeps the stated one — context/code/formula-and-io.md.
|
||||
kind: CategoryKind,
|
||||
}
|
||||
|
||||
/// One `## View:` section as pass 1 read it.
|
||||
@@ -60,16 +63,21 @@ impl ParsedFile {
|
||||
name,
|
||||
items,
|
||||
group_parents,
|
||||
kind,
|
||||
} = pc;
|
||||
match self.categories.iter_mut().find(|e| e.name == name) {
|
||||
Some(existing) => {
|
||||
existing.items.extend(items);
|
||||
existing.group_parents.extend(group_parents);
|
||||
if kind != CategoryKind::Regular {
|
||||
existing.kind = kind;
|
||||
}
|
||||
}
|
||||
None => self.categories.push(PCategory {
|
||||
name,
|
||||
items,
|
||||
group_parents,
|
||||
kind,
|
||||
}),
|
||||
}
|
||||
}
|
||||
@@ -164,9 +172,14 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
|
||||
name: section_title(&mut inner, "category_section")?,
|
||||
items: Vec::new(),
|
||||
group_parents: Vec::new(),
|
||||
kind: CategoryKind::Regular,
|
||||
};
|
||||
for entry in inner {
|
||||
match entry.as_rule() {
|
||||
Rule::kind_line => {
|
||||
let kw = next(&mut entry.into_inner(), "kind_line")?;
|
||||
pc.kind = parse_kind_kw(kw.as_str())?;
|
||||
}
|
||||
Rule::item_list => {
|
||||
for name_pair in entry.into_inner() {
|
||||
pc.items.push((extract_name(name_pair)?, None));
|
||||
@@ -185,6 +198,15 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
|
||||
Ok(pc)
|
||||
}
|
||||
|
||||
/// The on-disk keyword for a category kind, exhaustively — the reader's half
|
||||
/// of the `kind: label` line `format.rs` writes.
|
||||
fn parse_kind_kw(kind: &str) -> Result<CategoryKind> {
|
||||
match kind {
|
||||
"label" => Ok(CategoryKind::Label),
|
||||
_ => bail!("Unknown category kind: {kind}"),
|
||||
}
|
||||
}
|
||||
|
||||
/// The on-disk keyword for an aggregation, exhaustively — the reader's half of
|
||||
/// `format.rs::agg_keyword`.
|
||||
fn parse_agg_kind(kind: &str) -> Result<AggFunc> {
|
||||
@@ -361,7 +383,10 @@ fn walk_file(file: Pair<'_, Rule>) -> Result<ParsedFile> {
|
||||
|
||||
/// Register one category's items and groups, in file order.
|
||||
fn build_category(wb: &mut Workbook, pc: &PCategory) -> Result<()> {
|
||||
wb.add_category(&pc.name)?;
|
||||
match pc.kind {
|
||||
CategoryKind::Label => wb.add_label_category(&pc.name)?,
|
||||
_ => wb.add_category(&pc.name)?,
|
||||
};
|
||||
let cat = wb
|
||||
.model
|
||||
.category_mut(&pc.name)
|
||||
@@ -490,6 +515,7 @@ mod grammar_coverage {
|
||||
Rule::section => "silent: hoisted into file",
|
||||
Rule::category_section => "walk_category_section",
|
||||
Rule::item_list | Rule::grouped_item | Rule::group_hierarchy => "walk_category_section",
|
||||
Rule::kind_line | Rule::kind_kw => "walk_category_section",
|
||||
Rule::category_entry => "silent: hoisted into category_section",
|
||||
Rule::formulas_section | Rule::formula_line | Rule::formula_raw => {
|
||||
"walk_formulas_section"
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
use super::test_helpers::{coord, two_cat_model};
|
||||
use super::{format_md, parse_md};
|
||||
use crate::formula::parse_formula;
|
||||
use crate::model::category::Group;
|
||||
use crate::model::category::{CategoryKind, Group};
|
||||
use crate::model::cell::CellValue;
|
||||
use crate::view::Axis;
|
||||
use crate::workbook::Workbook;
|
||||
@@ -1158,3 +1158,79 @@ fn parse_md_reads_a_formula_with_no_format_as_none() {
|
||||
let loaded = parse_md(&format_md(&m)).unwrap();
|
||||
assert_eq!(loaded.model.formulas()[0].format, None);
|
||||
}
|
||||
|
||||
// ── Category kinds (improvise-dy3j) ──────────────────────────────────────
|
||||
|
||||
/// The kind is a parsed field, not a spelling the file happens to carry.
|
||||
/// Evidence: `context/test-evidence/formula-and-io.md`.
|
||||
#[test]
|
||||
fn a_label_category_survives_the_round_trip_as_a_label() {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_label_category("Note").unwrap();
|
||||
|
||||
let loaded = parse_md(&format_md(&wb)).unwrap();
|
||||
assert_eq!(
|
||||
loaded.model.category("Note").unwrap().kind,
|
||||
CategoryKind::Label
|
||||
);
|
||||
// The control that keeps the assertion above from passing vacuously: a
|
||||
// reader making every category a Label would clear it and fail here.
|
||||
assert_eq!(
|
||||
loaded.model.category("Region").unwrap().kind,
|
||||
CategoryKind::Regular
|
||||
);
|
||||
}
|
||||
|
||||
/// A label sits outside `MAX_CATEGORIES`, so a document at the limit plus one
|
||||
/// label must reload. Evidence: `context/test-evidence/formula-and-io.md`.
|
||||
#[test]
|
||||
fn a_document_at_the_category_limit_plus_a_label_reloads() {
|
||||
let mut wb = Workbook::new("T");
|
||||
for i in 0..12 {
|
||||
wb.add_category(format!("C{i}")).unwrap();
|
||||
}
|
||||
wb.add_label_category("Note").unwrap();
|
||||
|
||||
let loaded = parse_md(&format_md(&wb)).expect("a label must not count against the limit");
|
||||
assert_eq!(
|
||||
loaded.model.category("Note").unwrap().kind,
|
||||
CategoryKind::Label
|
||||
);
|
||||
assert_eq!(loaded.model.regular_category_names().len(), 12);
|
||||
}
|
||||
|
||||
/// A file written before the `kind:` line existed reads its categories as
|
||||
/// `Regular` — the compatible reading, and what the writer still omits.
|
||||
#[test]
|
||||
fn a_category_section_with_no_kind_line_loads_as_regular() {
|
||||
let text = "v2025-04-09\n\
|
||||
# Budget\n\
|
||||
\n\
|
||||
## Category: Region\n\
|
||||
- North, South\n";
|
||||
let loaded = parse_md(text).unwrap();
|
||||
assert_eq!(
|
||||
loaded.model.category("Region").unwrap().kind,
|
||||
CategoryKind::Regular
|
||||
);
|
||||
}
|
||||
|
||||
/// The loader seats a label off every pivot axis, which is the view half of
|
||||
/// the same fact (improvise-cj4u).
|
||||
#[test]
|
||||
fn a_loaded_label_category_is_not_on_a_pivot_axis() {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_label_category("Note").unwrap();
|
||||
wb.create_view("Other");
|
||||
|
||||
let loaded = parse_md(&format_md(&wb)).unwrap();
|
||||
for (name, view) in &loaded.views {
|
||||
assert_eq!(
|
||||
view.axis_of("Note"),
|
||||
Axis::None,
|
||||
"view {name} pivoted a label category"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user