Triage open/closed issues: parser fixes + opt-in transaction grouping#124
Conversation
- #121: resolve entry_date across year boundaries instead of leaving it in the value date's year; guessed_entry_date kept as an alias. - #111: accept customer references longer than 16 chars when a // bank reference follows (GLS / Atruvia); 16x cap retained when no // is present so banks that pack same-line extra data (e.g. Rabobank) are unaffected. - #117: accept supplementary details longer than 34 chars (Wise). All changes verified against the full fixture set with no change to existing parse output.
Some banks append a bare ' BIC'/' IBAN' label with no value to a ?2x segment. Segment keys are always two characters, so the previous '28D' check was dead code; match on the value instead and handle both BIC and IBAN. Replaces the fabricated test with realistic segment-pipeline input.
…st (#106) - #110: new opt-in transaction_boundary option (Transactions/parse) lets banks start a transaction on extra tag slugs (e.g. every :20:). Defaults to the legacy behaviour (only :61: starts a transaction); documented in the README. - #106: regression test confirming parsing works when sys.stdout/stderr are None (sandboxed environments); the refactor already removed the dependency.
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements and bug fixes to the MT940 parser, including an opt-in transaction_boundary feature to customize transaction grouping, support for longer customer references and supplementary details, and year-boundary correction for entry dates. It also fixes an issue where dangling 'BIC' or 'IBAN' labels are left in transaction details, and ensures parsing works in environments without stdout/stderr. The review feedback highlights two key improvements: first, ensuring that matching a boundary tag updates self.data with the tag's parsed result so subsequent transactions do not lose their references; second, adding a defensive check to handle a single string passed to transaction_boundary to prevent character-by-character iteration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3600304003
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR addresses several MT940 parsing edge cases reported in issues (#121, #111, #117, #109, #106) and introduces an opt-in mechanism to alter transaction grouping via configurable “boundary” tag slugs (issue #110), while aiming to preserve existing default behavior.
Changes:
- Fix year-boundary resolution for
entry_datein:61:parsing while keepingguessed_entry_dateas a backward-compatible alias. - Relax
:61:parsing constraints to support longcustomer_reference(when followed by//) and longextra_details, plus strip danglingBIC/IBANlabels from detail segments. - Add
transaction_boundaryoption tomt940.parse()/Transactions(...)and expand regression test coverage for the reported issues.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the new opt-in transaction grouping via transaction_boundary. |
| mt940/tags.py | Adjusts :61: regex and resolves entry_date across year boundaries. |
| mt940/processors.py | Strips dangling BIC / IBAN labels from detail segments. |
| mt940/parser.py | Plumbs transaction_boundary through the public parse() API. |
| mt940/models.py | Implements opt-in transaction boundaries during tag processing. |
| mt940_tests/test_processors_issue109.py | Adds end-to-end regression coverage for issue #109 segment handling. |
| mt940_tests/test_issues/test_long_references.py | Adds regression tests for issues #111 and #117 plus Rabobank BC behavior. |
| mt940_tests/test_issues/test_issue110_boundary.py | Adds tests for opt-in transaction boundary behavior. |
| mt940_tests/test_issues/test_issue106_streams.py | Adds regression test for parsing when sys.stdout/sys.stderr are None. |
| mt940_tests/test_entry_dates.py | Expands entry-date tests to cover forward/backward year boundary cases (#121). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- models: boundary tags with Transactions scope also update global data so a block's reference propagates to later :61: tags (gemini); accept a bare string for transaction_boundary instead of iterating it per-character. - tests: single mt940 import style (CodeQL); cover the new branches.
- #111: make long-customer-reference support opt-in via a new StatementGLS tag instead of relaxing the default regex, which could reclassify a // inside same-line details (e.g. a URL) as the bank reference (codex). Default :61: customer_reference cap restored; added a regression test for // in details. - models: handle Statement before transaction_boundary so a 'statement' slug can't bypass statement logic (copilot).
Goes through the open issues/PRs (and stale-closed ones) and implements everything safely fixable, without breaking backwards compatibility. Every parser change was verified against the full fixture set (parse →
JSONEncoder→ diff): zero change to existing output. New behaviour that would otherwise change output is opt-in.Bug fixes
entry_datenow resolves across year boundaries (e.g. value date Jan 2026 + entry date Dec → 2025-12-30) instead of staying in the value-date year.guessed_entry_datekept as an alias.customer_referencemay exceed 16 chars when a//bank reference follows; the 16x cap is kept when there's no//, so banks that pack same-line extra data (e.g. Rabobank) are unaffected.extra_detailsmay exceed 34 chars.BIC/IBANlabel (a label with no value) from detail segments; the old'28D'branch was dead code (segment keys are always 2 chars). IBAN handling now actually works.Robustness
sys.stdout/sys.stderrareNone(sandboxed/serverless). The dependency was already removed by the modernization; this locks it in.New (opt-in, BC-safe) feature
:61:tag (statement) and not:20:tag (transaction reference number) #110 —transaction_boundaryoption onmt940.parse()/Transactions(...): an iterable of tag slugs that each start a new transaction (e.g.{'transaction_reference_number'}to start one per:20:). Defaults to today's behaviour (only:61:starts a transaction). Documented in the README. This is the configurable "dialect"-style hook requested in the issue, rather than a default change (which previously broke ABN AMRO,transaction_referenceonly present in the first transaction. #26).Verification
toxgreen: py310–313, lint, pyright, mypy, pyrefly, docs, coverage 100%, codespell, repo-review.Fixes #121
Fixes #111
Fixes #117
Fixes #109
Fixes #110
Fixes #106