fix(rss): read an element's whole text, not only its first node - #2424
Closed
kevin (kevin9327) wants to merge 2 commits into
Closed
fix(rss): read an element's whole text, not only its first node#2424kevin (kevin9327) wants to merge 2 commits into
kevin (kevin9327) wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Elements beginning with markup no longer preserve the stated previous behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes RSS/Atom extraction so adjacent text and CDATA nodes are preserved.
Changes:
- Joins direct text and CDATA children.
- Adds RSS and Atom regression tests.
File summaries
| File | Description |
|---|---|
_rss_converter.py |
Updates element text extraction. |
test_rss_converter.py |
Adds CDATA regression coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+343
to
+347
| parts = [ | ||
| child.data | ||
| for child in nodes[0].childNodes | ||
| if child.nodeType in (Node.TEXT_NODE, Node.CDATA_SECTION_NODE) | ||
| ] |
Member
|
Addressed alternatively in #2432 |
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
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.
What this changes
An RSS item body written as a pretty-printed CDATA section is dropped. The item keeps its
heading; the content underneath it disappears.
That layout is ordinary in hand-written and CMS-generated feeds, and it is what WordPress
emits for
content:encoded.Why
_get_data_by_tag_namereads one node:An element's text is not necessarily one node. The example above reaches the parser as three
children -- a text node holding
"\n ", the CDATA section, then another text node -- sofirstChildis the indentation and the value is never read. The same happens whenever aCDATA section sits next to text:
_get_data_by_tag_namebackstitle,description,pubDateandcontent:encodedon theRSS side and
title,updated,summaryandcontenton the Atom side, so every one ofthem truncates at the first CDATA boundary.
Fix
Join the element's own text and CDATA children instead of reading only the first one. This is
what
_collect_node_textin_epub_converteralready does for the same minidom shape.The join is deliberately not recursive: only the element's direct text and CDATA children are
read, which is exactly the set
firstChildwas sampling from. Element children keep behavingas before --
<summary type="xhtml">is still handled by_get_atom_contentbefore thisfunction is reached, and an element whose first child is markup still returns
None.Tests
Added to
packages/markitdown/tests/test_rss_converter.py:<description>whose CDATA sits on its own line<title>with a CDATA section in the middle of the text<summary type="html">whose CDATA sits on its own line<description>that is nothing but a CDATA section, which already worked<description>at all, which must stay absent rather than becomean empty string
Against unmodified
main:With the fix:
How I tested
Windows 11, Python 3.12, editable install of
packages/markitdown[all].Those 18 are unchanged by this PR:
maingives18 failed, 426 passed, 4 skippedon thismachine before any edit, and the 5 added tests account for the difference. They are the CLI
stdout-encoding, Windows file-URI and speech-transcription tests that need a UTF-8 console,
a case-sensitive path and network/ffmpeg.
black --checkclean on both files.