fix: keep a section's own heading when its TOC anchor is nested inside it (#1369) - #1402
Conversation
dgunning
left a comment
There was a problem hiding this comment.
Thanks, this is a careful PR and the Google FY2004 result reproduces exactly (0/18 → 18/18, Item 15 gets both cells). Two things before it can merge:
1. The one CI failure is the fix working. test_issue_1345_shared_page_anchor.py::test_prospectus_trailing_section_markdown_stops_at_financial_statements now gets 1982 instead of 1934; the extra 48 chars are the section's own **Where You Can Find Additional Information ** heading. Please re-pin it with a comment saying so.
2. The climb reaches modern filings too. I compared Section.markdown() for every section in tests/fixtures/html (82 filings, 1,457 sections) on main vs this branch: 53 sections only gained their heading, as intended, but 34 sections in 12 modern filings changed in other ways (MSFT 10-K/10-Q, ORCL, TALO, HUBS, GS, MS, SO, ABNB). For example:
- MSFT 10-K Item 6:
**ITEM 6. [R**/**ESERVED]**/PART IIon separate lines →**ITEM 6\. \[R** **ESERVED\]** PART IIon one - a 2010 20-F (CIK 1464165): bold
**Item 4.**/**Information on the Company**→ plainItem 4. Information on the Company - page-footer and running-header lines disappearing from the middle of GS Item 1A and MS Item 5
In these layouts the anchor is the first child of a wrapper that also holds the following content, so _start_block climbs to that wrapper, and the whole block is cloned as one top-level element that the markdown renderer lays out differently. Could you narrow the climb (e.g. only through inline wrappers like b/font/span/a/u/i plus the one block that holds the heading; Item 15 still needs to reach the <table>) and include a before/after over tests/fixtures/html where every changed section is a pure heading prefix?
Minor, optional: on that ABNB section markdown() now includes the heading while text() (1,910 chars both before and after) does not, so it may be worth deciding whether the two should agree.
The trailing empty <p><b></b></p> on the end side is fine as a follow-up.
|
Thanks for the careful review and for running the full fixture comparison. Understood on both points. I'll re-pin the ABNB test with a comment, narrow the climb to inline wrappers plus the one block that holds the heading (keeping the |
|
Thanks for the review. Both points are addressed in the latest commits, and I've merged current 1. ABNB test re-pinned. 2. Climb narrowed.
Before/after over The two "changed otherwise" are ABNB's Google FY2004 10-K still gives 18 of 18, and Item 15 is Tests. Added three cases to the regression file: a This isn't limited to pre-2010 filings (ABNB's 2020 filings have the same layout), so I've reworded the changelog fragment. On |
What this changes
On filings the TOC anchor is nested inside the heading it marks (
<p><b><a name="item2"></a>ITEM 2. PROPERTIES</b></p>), soSection.markdown()returned each TOC-detected section without its own heading. Collection now starts at the outermost block that begins with the start anchor, so the heading is kept. Filings whose anchors stand on their own are unchanged.Fixes #1369
Verification
tests/issues/regression/, and its docstring links the issue or beadchangelog.d/<id>.<section>.mdfragment, not an edit toCHANGELOG.mdThe regression test uses inline HTML and runs offline, so I have not ticked the ground-truth box. I checked the real filing by hand instead; see below.
6.0
docs/upgrade/6.0.mdsays what users see now and what to do insteadWorking context (optional)
I worked on this with an AI assistant (Claude).
Plan. Mirror
_truncate_aton the start side, as the issue suggests._start_blockclimbs from the start anchor while it is the first content of each enclosing element (no text and no element before it), stopping short of<body>, and returns the outermost such element.collect_range_elementsuses it asstart_elementwhen the caller passed none, so the existing start-at-element path does the rest.Evidence.
HTMLParser(ParserConfig(form="10-K")), first line of each TOC-detected section'smarkdown(): 0 of 18 start with their "ITEM N." heading onmain, 18 of 18 on this branch. Item 15, split across two table cells, now returns both cells.mainand passes here.tests/documents/test_section_slicer.pyandtests/issues/regression/test_issue_826.pypass: 13 passed.ruff check edgar/documents/utils/section_slicer.pyandcheck_regression_provenance.pypass.Ruled out.
_start_blockreturnsNone, leaving collection unchanged, when the anchor is a sibling of its heading, when text precedes the anchor inside its block, and when another element precedes it.Not included. The end side still leaves an empty
<p><b></b></p>where the next heading was truncated, as the issue notes. Happy to handle that here or in a follow-up.