feat(eval): implement holiday support for NETWORKDAYS and WORKDAY#36
feat(eval): implement holiday support for NETWORKDAYS and WORKDAY#36TyrGo wants to merge 1 commit intoPSU3D0:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds correct holiday exclusion support to the date/workday builtins so callers supplying a holidays argument get accurate results (matching typical spreadsheet behavior), and extends coverage via unit + JSON integration tests.
Changes:
- Add holiday parsing/coercion helpers (
collect_holidays,literal_to_serial) and use aHashSet<NaiveDate>for O(1) holiday lookups. - Wire
NETWORKDAYSandWORKDAYto exclude/skip holidays in addition to weekends. - Update docs/examples and add Rust unit tests + JSON formula runner tests for holiday scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crates/formualizer-eval/src/builtins/datetime/weekday_workday.rs |
Implements holiday collection/coercion and uses it in NETWORKDAYS/WORKDAY, plus adds unit tests and doc updates. |
tests/formula_tests/date_time.json |
Adds formula-runner integration tests validating holiday handling for scalar and range inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| vec![] | ||
| }; | ||
| let holidays = collect_holidays(args, 2)?; |
There was a problem hiding this comment.
collect_holidays(args, 2)? will propagate coercion/serial conversion errors as a returned Err from eval, which the formula test runner treats as an evaluation failure rather than an in-cell #VALUE!/#NUM! result. Since the holidays argument uses ArgSchema::any() (no coercion/validation), consider catching Err(e) here and returning Ok(CalcValue::Scalar(LiteralValue::Error(e))) so invalid holiday inputs and holiday-range errors behave like regular Excel errors instead of aborting evaluation.
| let holidays = collect_holidays(args, 2)?; | |
| let holidays = match collect_holidays(args, 2) { | |
| Ok(h) => h, | |
| Err(e) => { | |
| return Ok(CalcValue::Scalar(LiteralValue::Error(e))); | |
| } | |
| }; |
| // TODO: Implement holiday array support | ||
| // Holidays parameter is currently accepted but ignored. | ||
| let holidays: Vec<NaiveDate> = Vec::new(); | ||
| let holidays = collect_holidays(args, 2)?; |
There was a problem hiding this comment.
Same issue as NETWORKDAYS: collect_holidays(args, 2)? can cause WORKDAY to return an Err(ExcelError) for bad holiday inputs (text that can’t be parsed, out-of-range serials, errors inside a holiday range), which surfaces as an evaluation failure instead of an in-cell error value. Handle Err(e) by returning Ok(CalcValue::Scalar(LiteralValue::Error(e))) at this callsite.
| let holidays = collect_holidays(args, 2)?; | |
| let holidays = match collect_holidays(args, 2) { | |
| Ok(h) => h, | |
| Err(e) => { | |
| return Ok(CalcValue::Scalar(LiteralValue::Error(e))); | |
| } | |
| }; |
There was a problem hiding this comment.
I considered this, but every other error-returning call in this file uses ? propagation (coerce_to_serial(&args[0])?, coerce_to_int(&args[1])?, etc.). Wrapping just collect_holidays in a match would be inconsistent with the established pattern. If error wrapping should be preferred over ? propagation, that seems like a codebase-wide decision rather than something to change for one callsite.
crates/formualizer-eval/src/builtins/datetime/weekday_workday.rs
Outdated
Show resolved
Hide resolved
crates/formualizer-eval/src/builtins/datetime/weekday_workday.rs
Outdated
Show resolved
Hide resolved
4f9cca2 to
32d56c7
Compare
The holidays parameter (third argument) was accepted but silently ignored in both NETWORKDAYS and WORKDAY, producing incorrect results when callers supplied holiday dates. Changes: - Add collect_holidays() helper that parses the holidays argument from a cell range, inline array constant, or single scalar value into a HashSet<NaiveDate> for O(1) lookups during day iteration. - Add literal_to_serial() helper for coercing LiteralValue variants to date serials within flattened holiday arrays. - Wire both NETWORKDAYS and WORKDAY eval bodies to use collect_holidays(), replacing the empty Vec placeholders. - Update docstrings and inline examples to reflect correct behavior. - Add 10 unit tests covering: no holidays, single scalar holiday, array holidays, holiday on weekend (no effect), reversed date range, negative workday offset, holiday on start date, and native Date literal holidays. - Add 6 JSON formula integration tests for range and scalar holiday inputs via the formula test runner.
32d56c7 to
6e315d9
Compare
|
Covered by recent merge. Closed. |
The holidays parameter (third argument) was accepted but silently ignored in both NETWORKDAYS and WORKDAY, producing incorrect results when callers supplied holiday dates.
Changes: