networkdays, networkdays.intl #33
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BrianHung
changed the title
Owner
Author
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
cursor review |
Owner
Author
|
bugbot run cursor review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
bugbot run cursor review |
BrianHung
added a commit
that referenced
this pull request
Jul 28, 2025
Resolved conflicts by keeping our working implementation while incorporating the test files and documentation from the PR.
BrianHung
added a commit
that referenced
this pull request
Jul 28, 2025
Consolidated duplicate helper functions introduced across PRs #35, #36, #41, #33: ✅ **Holiday/Date Array Processing**: - Merged get_holiday_set() (PR #41) and get_array_of_dates() (PR #33) - New: process_date_array() - unified date array processing - New: dates_to_holiday_set() - converter for backward compatibility - Removed ~100 lines of duplicate code ✅ **Weekend Pattern Processing**: - Merged weekend_from_arg() (PR #41) and parse_weekend_pattern() (PR #33) - New: parse_weekend_pattern_unified() - consolidated weekend parsing - Legacy wrapper: weekend_from_arg() for backward compatibility - Handles both numeric codes (1-7, 11-17) and string patterns ("0110000") **Impact**: Reduced code duplication by ~200 lines while maintaining full functionality **Functions using consolidated helpers**: WORKDAY, WORKDAY.INTL, NETWORKDAYS, NETWORKDAYS.INTL
BrianHung
added a commit
that referenced
this pull request
Jul 28, 2025
Found and eliminated the biggest remaining duplication pattern from PRs #35, #36, #41, #33: ✅ **Date Validation Pattern** (29+ occurrences → consolidated): - Old: Repeated `from_excel_date` + identical error handling in every function - New: `validate_and_convert_date_serial()` - single source of truth - New: `get_and_validate_date_serial()` - handles common get_number + validate pattern **Functions Updated** (examples): - fn_weekday: 9 lines → 3 lines - fn_weeknum: 9 lines → 3 lines - fn_workday: 9 lines → 3 lines **Potential Impact**: - 29+ functions with identical 9-line patterns can now use 3-line helpers - Estimated savings: ~180 lines of duplicate code - Single location for date validation logic changes - Consistent error handling across all date functions **Pattern Eliminated**: \`\`\`rust // Before (repeated 29+ times): let serial = match self.get_number(&args[0], cell) { ... }; let date = match from_excel_date(serial) { Ok(d) => d, Err(_) => return CalcResult::Error { ... } }; // After (reusable helper): let (_serial, date) = match self.get_and_validate_date_serial(&args[0], cell) { ... }; \`\`\`
Closed
BrianHung
added a commit
that referenced
this pull request
Jul 29, 2025
Owner
Author
|
Consolidating into #50. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
NETWORKDAYSandNETWORKDAYS.INTLimplementationsTesting
cargo test -p ironcalc_base --lib --quiethttps://chatgpt.com/codex/tasks/task_e_68658c30afb08327965806a8efd37bd1