fix(preprocess): parse shared exchange ticker lists - #129
Conversation
keli-wen
left a comment
There was a problem hiding this comment.
Requesting changes. Thanks for taking on #108 — the intent is right, but the shared-prefix extension as written is a net regression, and I don't think this change should carry a new example or a design-doc rewrite. Details below.
Blocking: the shared-prefix scan over-captures ordinary prose
The continuation loop keys off match.end() and then searches for the next ) anywhere in the text (scan_text.find(")", match.end())). When the main regex already matched a balanced (EXCHANGE: SYMBOL) — it consumed its own closing paren — the loop still runs and scans past that paren into unrelated following text. Because _SHARED_EXCHANGE_SYMBOL_RE is compiled with re.IGNORECASE, an ordinary lowercase word right after a (EXCHANGE: SYMBOL), is captured as a ticker.
This flows straight into preprocess_news_document (ticker_hints, news.py:330), so it pollutes real output. On current master the input below yields one hint; on this branch it yields a bogus second one:
# (symbol, exchange, raw) projected from each returned NewsTickerHint
>>> extract_exchange_ticker_hints("Shares of (NYSE: IBM), and (NASDAQ: AAPL) rose today.")
('IBM', 'NYSE', '(NYSE: IBM)')
('AND', 'NYSE', 'NYSE: AND') # <- "and" captured as ticker "AND"
('AAPL', 'NASDAQ', '(NASDAQ: AAPL)')
The trigger — a balanced (EXCH: SYM), followed by any later ) — is extremely common in PR-wire prose, so this regresses far more documents than the ~1 warrant of recall that #108 estimates it recovers. The added tests only assert the five happy-path shapes from the issue and never exercise this case, so CI stays green while the regression ships.
Related smell: raw is meant to be the literal matched substring (provenance), but continuation members set raw=f"{exchange}: {symbol}" — a string that never appears in the source (NYSE: EVEXW is reconstructed) — while the first member keeps a dangling (NYSE: EVEX. The tests encode both, which locks that broken raw contract in.
This change should not add an example
extract_exchange_ticker_hints already exists and is already demonstrated end-to-end by examples/preprocess/01_news_pr_wire.py. Extending an existing operation's parsing coverage is a fix, not a new public operation, so it should ship regression tests rather than a second example file. Please drop examples/preprocess/02_news_ticker_lists.py. We'll also tighten the contributor guidance so the "ships with an example" expectation is scoped to new public operations, not edge-case extensions.
The contexts/ doc change is questionable here too
Whether a parsing edge-case should rewrite contexts/design/flow/news.md is debatable — unless this actually changes a documented contract, the design doc probably shouldn't move. It is also the only real merge conflict with master right now (#130 rewrote that file), so dropping the doc change both narrows scope and clears the conflict.
Suggested path
Narrow this to what #108 is actually worth: keep the existing simple/balanced capture and add regression tests that pin the boundary — the first symbol of a shared-prefix list is captured, and (EXCH: SYM), ...) does not over-capture. If we would rather not carry the heuristic at all, deprecating ticker_hints is a reasonable alternative; happy to discuss which way you'd prefer.
Note: I pushed a small chore: normalize CRLF line endings to LF commit to this branch. The files were saved with CRLF, which made the diff look like a full-file rewrite; it now reads as ~+89/-4 against the merge base.
94931e5 to
638d16a
Compare
|
Thanks for the detailed review — the over-capture and provenance concerns were valid. I rebuilt the branch on the latest master and narrowed the PR to the parser plus focused regression tests only. The extra example and design-doc changes are gone. The continuation scan now runs only when the initial parenthesized exchange match has not already consumed its closing ), and continuation symbols require uppercase ticker syntax. This prevents (NYSE: IBM), and (NASDAQ: AAPL) from producing a bogus AND hint. Continuation I added regression coverage for the valid shared-prefix list and the balanced-parenthesis/prose, conjunction, semicolon, and unsupported-exchange boundaries. Verification:
The PR is now mergeable and contains only two changed files. Please take another look when convenient. |
|
Thanks again for the careful review — it was genuinely helpful, especially the concrete ordinary-prose regression and the provenance concern. One remaining contract question before I finalize this: for a shared-prefix group such as (NYSE: EVEX, EVEXW), should
The current revision avoids reconstructed text, but the first hint still retains the existing partial raw value (NYSE: EVEX, while the continuation stores , EVEXW. I’d prefer to align on the intended provenance contract rather than lock that behavior into the tests. Happy to adjust it based on your preference. |
|
Thanks again for the concrete review. I revisited the patch against each point and pushed one more bounded revision. It now extends only an unmatched parenthesized exchange group containing uppercase comma-separated symbols, and the continuation must end at For provenance, I chose a source-faithful contract rather than reconstructing text: every hint produced by a valid shared group stores the same complete literal parenthesized group in Focused verification is 23 passed plus 11 subtests; formatting, lint, changed-module type checking, and all 8 import contracts pass. The full Windows run reached 412 passed and 85.47% coverage; its 18 failures are the existing Windows SQLite temporary-file cleanup and LiteParse PDF cases, none in this parser. I have kept the PR as draft, so there is no urgency; feedback when convenient is welcome. |
Summary
Extend extract_exchange_ticker_hints only for the evidenced shared-prefix comma-list shape while preserving the existing balanced/simple capture behavior.
The parser now:
The PR remains limited to the parser and focused regression tests. The earlier example and design-document changes are removed.
Why
Issue #108 identifies one supported-symbol gain in the observed PR Newswire window: EVEXW in (NYSE: EVEX, EVEXW; B3: EVEB31). A broad continuation scan is not justified by that small gain. This revision implements the narrow grammar represented by the evidence and rejects prose, conjunction, unsupported-exchange, and non-exchange semicolon boundaries.
Review Follow-up
This revision addresses each blocking point from the requested-changes review:
Verification
Fixes #108.