Merge pull request #150 from fiddlerwoaroof/fix/yss5-date-mismatch-warning
fix(io): a date that does not match the detected format is reported, not dropped in silence (improvise-yss5)
This commit is contained in:
Ed Langley
GitHub
@@ -1354,3 +1354,73 @@ 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.
|
||||
|
||||
### import::wizard::tests::build_model_warns_about_values_that_do_not_match_the_date_format
|
||||
|
||||
improvise-yss5, and the ticket said plainly it had **not** been reproduced. It
|
||||
is real and it is narrower than it reads. `extract_date_component` returns
|
||||
`None` for a value the detected format cannot parse, and the caller's `if let
|
||||
Some(..)` simply does not push the derived coordinate — so the row silently
|
||||
leaves the derived `Date_Month`/`Date_Year` category with nothing reported, the
|
||||
same shape improvise-2cw9/jc95/5hfh closed elsewhere in this path.
|
||||
|
||||
**Through the shipped CLI and TUI wizard it is unreachable**, and the reason is
|
||||
worth writing down because it is a property of one function.
|
||||
`detect_date_format` accepts a format only when **every** distinct non-empty
|
||||
sample parses, and `extract_date_component` then makes the identical
|
||||
`NaiveDate::parse_from_str` call — so a mismatch requires the format to come
|
||||
from somewhere other than detection over the same values. That door is
|
||||
`ImportPipeline`'s `pub proposals`: a library caller may set `date_format`
|
||||
itself, and the test does. `a_column_of_mixed_date_formats_is_not_proposed_as_a_time_category`
|
||||
guards the reachability claim rather than leaving it as prose, per
|
||||
`doctrine(an_unreachability_claim_is_only_as_wide_as_the_routes_enumerated)`.
|
||||
|
||||
The fixture derives **two** columns (Month and Year) from one `Date` whose three
|
||||
values include two the format rejects, and the assertion is on the full warning
|
||||
vector rather than on a substring — so "one located warning per derived column"
|
||||
is separable from "one line saying some dates did not parse".
|
||||
|
||||
Three rivals, each caught at a different assertion:
|
||||
|
||||
- **Dropping the `count > 0` filter** — warn unconditionally — passed the entire
|
||||
suite before `extracting_a_date_component_from_values_that_all_parse_warns_about_nothing`
|
||||
was added, because no existing test enabled date extraction *and* asserted
|
||||
silence. That is the one-directional fault
|
||||
`context/design-principles.md` §7 names: a check that only asserts the warning
|
||||
appears is satisfied by a writer that always warns. It now fails there.
|
||||
- **Tallying into one counter instead of one per extraction** fails the vector
|
||||
comparison, which a single-derived-column fixture could not see.
|
||||
- **Weakening `detect_date_format` from `all` to `any`** fails the reachability
|
||||
guard here and three analyzer tests, which is the evidence that guard is
|
||||
load-bearing and not decoration.
|
||||
|
||||
The fix reports and does not relocate: the row still lands with the coordinates
|
||||
it does have, exactly as before. Changing where such a row is placed is a
|
||||
behaviour change the ticket did not ask for.
|
||||
|
||||
### import::wizard::tests::a_row_dropped_for_a_missing_category_is_not_charged_to_the_date_tally
|
||||
|
||||
improvise-yss5, found by ChatGPT Codex reviewing PR #150 at `af7fae0e` and
|
||||
correct. The first fix incremented `unparsed_dates` while walking a record's
|
||||
categories, **before the record was known to survive that walk**. A row whose
|
||||
`Date` is unparseable and whose *later* required category is missing is skipped
|
||||
entirely — it never lands in the imported model — yet it had already been
|
||||
charged to the derived column's tally. So the warning counted a row nothing else
|
||||
in the import reports, and its value depended on the **order** the categories
|
||||
happen to be walked in.
|
||||
|
||||
The mismatches are now collected per record and merged only after
|
||||
`missing.is_none()`. The fixture only discriminates while `Date` precedes the
|
||||
missing category, so the test asserts that ordering explicitly rather than
|
||||
relying on it — a later change to proposal order would otherwise make the test
|
||||
pass by walking the missing category first and never reaching the bad date.
|
||||
|
||||
Before the fix it reported three warnings where one is correct, naming
|
||||
`Date_Month` and `Date_Year` for a row that does not exist in the model.
|
||||
Injecting the rival — merging the per-record tally *before* the `missing` check
|
||||
rather than after — reproduces exactly that and fails here.
|
||||
|
||||
This is the second time in this ticket that the interesting question was *which
|
||||
rows the count is over*. The first was one warning per derived column rather
|
||||
than one overall; this is one warning per **landed** row rather than per
|
||||
attempted one.
|
||||
|
||||
@@ -187,6 +187,9 @@ impl ImportPipeline {
|
||||
// value; a record is charged to the first category it is missing.
|
||||
let mut skipped_per_category = vec![0usize; categories.len()];
|
||||
let mut skipped_non_objects = 0usize;
|
||||
// Per-extraction tally of values the detected format could not parse. A
|
||||
// derived coordinate that is merely absent is improvise-yss5.
|
||||
let mut unparsed_dates = vec![0usize; date_extractions.len()];
|
||||
// Two rows sharing every coordinate are one cell; last-row-wins lost the
|
||||
// earlier one (improvise-2cw9). The map only deduplicates — gotcha 18.
|
||||
let mut cells: Vec<PendingCell> = vec![];
|
||||
@@ -196,6 +199,9 @@ impl ImportPipeline {
|
||||
if let Value::Object(map) = record {
|
||||
let mut coords: Vec<(String, String)> = vec![];
|
||||
let mut missing: Option<usize> = None;
|
||||
// Charged only once this record survives the walk: a row dropped
|
||||
// for a LATER missing category never lands (context/test-evidence).
|
||||
let mut unparsed_here: Vec<usize> = vec![];
|
||||
|
||||
for (cat_idx, cat_proposal) in categories.iter().enumerate() {
|
||||
let val = map.get(&cat_proposal.field).and_then(scalar_name);
|
||||
@@ -205,12 +211,18 @@ impl ImportPipeline {
|
||||
coords.push((cat_proposal.field.clone(), v.clone()));
|
||||
|
||||
// Extract date components from this field's value
|
||||
for (field, fmt, comp, derived_name) in &date_extractions {
|
||||
if *field == cat_proposal.field
|
||||
&& let Some(derived_val) = extract_date_component(&v, fmt, *comp)
|
||||
{
|
||||
wb.add_item(derived_name, &derived_val)?;
|
||||
coords.push((derived_name.clone(), derived_val));
|
||||
for (at, (field, fmt, comp, derived_name)) in
|
||||
date_extractions.iter().enumerate()
|
||||
{
|
||||
if *field != cat_proposal.field {
|
||||
continue;
|
||||
}
|
||||
match extract_date_component(&v, fmt, *comp) {
|
||||
Some(derived_val) => {
|
||||
wb.add_item(derived_name, &derived_val)?;
|
||||
coords.push((derived_name.clone(), derived_val));
|
||||
}
|
||||
None => unparsed_here.push(at),
|
||||
}
|
||||
}
|
||||
} else {
|
||||
@@ -223,6 +235,9 @@ impl ImportPipeline {
|
||||
skipped_per_category[cat_idx] += 1;
|
||||
continue;
|
||||
}
|
||||
for at in unparsed_here {
|
||||
unparsed_dates[at] += 1;
|
||||
}
|
||||
|
||||
// Attach label values as coords (missing labels become "").
|
||||
for lab in &labels {
|
||||
@@ -284,6 +299,7 @@ impl ImportPipeline {
|
||||
&skipped_per_category,
|
||||
skipped_non_objects,
|
||||
);
|
||||
warnings.extend(unparsed_date_warnings(&date_extractions, &unparsed_dates));
|
||||
warnings.extend(collision_warning(&collisions));
|
||||
|
||||
Ok((wb, warnings))
|
||||
@@ -329,6 +345,25 @@ fn scalar_name(v: &Value) -> Option<String> {
|
||||
(!name.trim().is_empty()).then_some(name)
|
||||
}
|
||||
|
||||
/// One warning per derived date column whose source value some row could not
|
||||
/// parse: the coordinate is absent, not wrong, so nothing else can report it.
|
||||
fn unparsed_date_warnings(
|
||||
date_extractions: &[(&str, &str, DateComponent, String)],
|
||||
unparsed: &[usize],
|
||||
) -> Vec<String> {
|
||||
date_extractions
|
||||
.iter()
|
||||
.zip(unparsed)
|
||||
.filter(|&(_, &count)| count > 0)
|
||||
.map(|((field, fmt, _, derived_name), count)| {
|
||||
format!(
|
||||
"column '{derived_name}' missing from {count} row(s); \
|
||||
'{field}' did not match the date format '{fmt}'"
|
||||
)
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Human-readable warnings for records that could not be placed in the grid.
|
||||
/// Empty when every record was imported.
|
||||
fn skip_warnings(
|
||||
@@ -939,6 +974,127 @@ mod tests {
|
||||
assert_eq!(val, Some(60.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_column_of_mixed_date_formats_is_not_proposed_as_a_time_category() {
|
||||
let raw = json!([
|
||||
{"Date": "01/15/2025", "Amount": 100.0},
|
||||
{"Date": "2025-02-05", "Amount": 200.0},
|
||||
]);
|
||||
let p = ImportPipeline::new(raw);
|
||||
let date = p
|
||||
.proposals
|
||||
.iter()
|
||||
.find(|p| p.field == "Date")
|
||||
.expect("Date");
|
||||
assert_eq!(
|
||||
date.date_format, None,
|
||||
"detect_date_format requires EVERY distinct sample to parse, which is why \
|
||||
the honest CLI/wizard route cannot hand build_model a value it will refuse"
|
||||
);
|
||||
}
|
||||
|
||||
/// A pipeline extracting Month and Year from `Date` under `%m/%d/%Y`.
|
||||
fn extracting_month_and_year(raw: serde_json::Value) -> ImportPipeline {
|
||||
use crate::import::analyzer::DateComponent;
|
||||
|
||||
let mut p = ImportPipeline::new(raw);
|
||||
for prop in &mut p.proposals {
|
||||
if prop.field == "Date" {
|
||||
prop.kind = FieldKind::TimeCategory;
|
||||
prop.accepted = true;
|
||||
prop.date_format = Some("%m/%d/%Y".to_string());
|
||||
prop.date_components.push(DateComponent::Month);
|
||||
prop.date_components.push(DateComponent::Year);
|
||||
}
|
||||
}
|
||||
p
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extracting_a_date_component_from_values_that_all_parse_warns_about_nothing() {
|
||||
let p = extracting_month_and_year(json!([
|
||||
{"Date": "01/15/2025", "Amount": 100.0},
|
||||
{"Date": "02/05/2025", "Amount": 200.0},
|
||||
]));
|
||||
let (_, warnings) = p.build_model_with_warnings().unwrap();
|
||||
assert!(
|
||||
warnings.is_empty(),
|
||||
"a clean import must stay silent: {warnings:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_row_dropped_for_a_missing_category_is_not_charged_to_the_date_tally() {
|
||||
// `Date` is walked before `Region`, so a bad date is seen first and the
|
||||
// record is only afterwards dropped for the missing `Region`.
|
||||
let mut p = extracting_month_and_year(json!([
|
||||
{"Date": "01/15/2025", "Region": "East", "Amount": 100.0},
|
||||
{"Date": "2025-02-05", "Amount": 200.0},
|
||||
]));
|
||||
for prop in &mut p.proposals {
|
||||
if prop.field == "Region" {
|
||||
prop.kind = FieldKind::Category;
|
||||
prop.accepted = true;
|
||||
}
|
||||
}
|
||||
let categories: Vec<&str> = p
|
||||
.proposals
|
||||
.iter()
|
||||
.filter(|p| {
|
||||
p.accepted && matches!(p.kind, FieldKind::Category | FieldKind::TimeCategory)
|
||||
})
|
||||
.map(|p| p.field.as_str())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
categories,
|
||||
vec!["Date", "Region"],
|
||||
"the fixture only discriminates while Date is walked FIRST"
|
||||
);
|
||||
|
||||
let (_, warnings) = p.build_model_with_warnings().unwrap();
|
||||
assert!(
|
||||
warnings.iter().all(|w| !w.contains("Date_Month")),
|
||||
"the row never landed, so no derived column lost it: {warnings:?}"
|
||||
);
|
||||
assert_eq!(
|
||||
warnings.len(),
|
||||
1,
|
||||
"only the skipped-record warning: {warnings:?}"
|
||||
);
|
||||
assert!(warnings[0].contains("Region"), "{}", warnings[0]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_model_warns_about_values_that_do_not_match_the_date_format() {
|
||||
let p = extracting_month_and_year(json!([
|
||||
{"Date": "01/15/2025", "Amount": 100.0},
|
||||
{"Date": "2025-02-05", "Amount": 200.0},
|
||||
{"Date": "2025-03-05", "Amount": 300.0},
|
||||
]));
|
||||
let (wb, warnings) = p.build_model_with_warnings().unwrap();
|
||||
let derived = wb
|
||||
.model
|
||||
.category("Date_Month")
|
||||
.expect("the derived category");
|
||||
assert_eq!(
|
||||
derived.items.keys().map(String::as_str).collect::<Vec<_>>(),
|
||||
vec!["2025-01"],
|
||||
"two of the three values did not parse, so they contribute no item"
|
||||
);
|
||||
// Two derived columns lost the same two rows, and each says so: a single
|
||||
// "some dates did not parse" line cannot be told from this one.
|
||||
assert_eq!(
|
||||
warnings,
|
||||
vec![
|
||||
"column 'Date_Month' missing from 2 row(s); 'Date' did not match the date \
|
||||
format '%m/%d/%Y'",
|
||||
"column 'Date_Year' missing from 2 row(s); 'Date' did not match the date \
|
||||
format '%m/%d/%Y'",
|
||||
],
|
||||
"one located warning per derived column"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_model_extracts_date_month_component() {
|
||||
use crate::import::analyzer::DateComponent;
|
||||
|
||||
Reference in New Issue
Block a user