Skip to content

fix(rss): separate entry dates from previous bodies - #2606

Open
Minh Vu (fallintoplace) wants to merge 1 commit into
microsoft:mainfrom
fallintoplace:fix/rss-entry-date-separators
Open

Minh Vu (fallintoplace) wants to merge 1 commit into
microsoft:mainfrom
fallintoplace:fix/rss-entry-date-separators

Conversation

@fallintoplace

Copy link
Copy Markdown

What

  • Keep RSS and Atom entry dates separate from the previous body.

Why

  • Missing or blank entry titles can produce First body.Published on: ....

Implementation

  • Add a separator before the date when the previous text has no newline.
  • Cover missing, empty and blank titles, plus dated and undated entries.

@kokokoXUY XU (kokokoXUY) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The PR inserts a blank line before the Published on: / Updated on: line when the text
emitted so far does not end with a newline (_rss_converter.py:186-189 for Atom,
:316-319 for RSS). That is the right place to fix the reported defect: for an entry
without a title, the previous entry's body was concatenated straight onto the date line
(First body.Published on: ...).

I reproduced the defect on the base commit, confirmed the fix on the head commit, and
then looked for inputs where the new guard does not fire.

What I verified

  1. Red/green on the repository tests. packages/markitdown/tests/test_rss.py from
    the PR head, run against both trees with only PYTHONPATH switched:

    • base 4cc9fa17 with the head test file: 12 failed, 184 passed
    • head 4d1506f6 with the same file: 196 passed
      Every failure is a test_feed_entry_dates_are_separated_from_previous_body[...dated...]
      case with a missing, empty or blank entry title. Base with its own test file: 164
      passed; head with its own file: 196 passed (32 new cases). No existing test regresses.
  2. Converter level, direct and public API. Synthetic RSS and Atom feeds converted
    through RssConverter().convert(...) and MarkItDown().convert_stream(...) give
    identical output on both trees:

    • base: '\n## First\nPublished on: Mon, 01 Jan 2024 00:00:00 GMT\nFirst body.Published on: Tue, 02 Jan 2024 00:00:00 GMT\nSecond body.'
    • head: '\n## First\nPublished on: Mon, 01 Jan 2024 00:00:00 GMT\nFirst body.\n\nPublished on: Tue, 02 Jan 2024 00:00:00 GMT\nSecond body.'
      Rendering the head output with markdown-it-py separates First body. and
      Published on: ... into two <p> elements, which is what the PR claims.
  3. Formatting. black 23.7.0 --check (the version pinned by the repository's
    pre-commit config) on both changed files: "2 files would be left unchanged". The base
    files are clean as well.

  4. Mergeability. Upstream main is still at the PR's base commit 4cc9fa17, so
    there is no drift; git merge-tree --write-tree main 4d1506f6 exits 0 (tree
    7ba939405ac347aafeb48d869ddff0fc9ac60f3b). It also merges cleanly with the other
    open RSS PR (#2564), which edits the same file.

Remaining gap (non-blocking, worth tracking)

The guard only fires when the emitted text does not end with a newline. Text that
ends with exactly one newline still leaves the date inside the previous paragraph, and
the converter itself produces such text in two places:

  • RSS channel description, _rss_converter.py:307: md_text += f"{channel_description}\n"
  • Atom feed subtitle, _rss_converter.py:177: md_text += f"{subtitle}\n"

Reproduction (channel with a description, first item has no title, item carries a pubDate):

<rss version="2.0"><channel><title>Feed</title>
<description>Channel description.</description>
<item><pubDate>Tue, 02 Jan 2024 00:00:00 GMT</pubDate>
<description>Body.</description></item>
</channel></rss>

Markdown produced on the head commit:

# Feed
Channel description.
Published on: Tue, 02 Jan 2024 00:00:00 GMT
Body.

CommonMark renders that as one paragraph:
<h1>Feed</h1> <p>Channel description. Published on: Tue, 02 Jan 2024 00:00:00 GMT Body.</p>

  • the same "date glued to the previous text" symptom the PR sets out to remove. The Atom
    subtitle case is identical (Updated on: lands inside the subtitle paragraph). Both
    shapes are byte-identical on base and head, so this is an untouched pre-existing case,
    not a regression.

Two one-line remedies both have a cost, which is why I am not asking for either of them
in this PR:

  • Require a blank line in both guards (not md_text.endswith("\n\n")): fixes the two
    cases above, but also inserts a blank line after an entry heading. In a copy of the head
    tree that variant fails 24 tests in test_rss.py, 16 of them the new titled cases,
    which expect ...First body.\n## Second\nPublished on:....
  • Append a blank line where the channel description and the Atom subtitle are emitted:
    fixes both cases above, but changes expectations for existing feeds with a subtitle or
    channel description (22 failed, 174 passed in test_rss.py).

A separator helper that knows whether the last block was a paragraph or a heading would
cover all of it, but that is a larger change than this PR. Adding one regression case for
the channel-description/subtitle shape, or filing a follow-up issue, would keep the gap
from being forgotten.

Other observations (non-blocking)

  • No CI has run for this head: the pre-commit (37209391490) and tests (37209391468)
    workflow runs are both action_required (first-time contributor), and the only check
    run on 4d1506f6 is license/cla (success). The numbers above are local runs, not CI.
  • The new test covers missing / empty / blank / titled entry titles, dated and undated
    entries, and two body shapes, on both RSS and Atom, and asserts the direct converter and
    the public API path - good coverage of the reported shape.
  • The guard is duplicated in the two parsers. If the follow-up above is ever done, one
    shared helper would keep the two paths from diverging.
  • The PR body links no issue. Search turns up no open issue for this specific symptom, so
    a short reproduction in the body (or a new issue) would help whoever reads this later.

How this was produced

All findings come from running the head and base trees locally (Python 3.13.12, pytest,
markdown-it-py 4.2.0 for rendering, black 23.7.0, git merge-tree). The two variants
discussed above were built by copying the head worktree and applying the one-line edits
there; the PR branch itself was not modified. I used AI assistance (DeepSeek) for the
analysis and for drafting this text, and executed and read back every command and output
quoted here.

Verdict

The change is correct, minimal, tested, and a clear improvement over the current
behaviour, and I would merge it. The single-newline case is a real remaining gap, but
closing it needs changes beyond this PR's scope, so a documented follow-up is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants