Complete RSS/OPML output metadata #35
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-28-feed-opml-metadata"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #28
Two output-metadata additions (the width/height dimensions were decided out — see the review thread):
<head><dateModified>— current UTC timestamp in RFC 822 format (locale-independent viaemail.utils.format_datetime), so aggregators can tell when the index last changed.write_opml()takes averboseflag and warns when a channel directory has nofeed.xmlat OPML generation time (threaded through from the CLI's--verbose).Covered by Tryke tests (red → green):
dateModifiedformat, and the verbose warning (warn + silent paths).WuMing
Found 3 issue(s). See inline comments below.
@ -98,3 +98,3 @@print(str(opml_path))else:write_opml(parent_dir, channels, base_url)write_opml(parent_dir, channels, base_url, verbose=verbose)tests [LOW]
The CLI now threads
verboseintowrite_opml, but no test in the diff exercises this wiring. The new unit tests coverwrite_opmldirectly, so a regression that drops this argument would go undetected. Add a CLI-level test (e.g., in the existing CLI opml.xml integration block) that runs with--verboseand a channel missingfeed.xml, asserting the warning is printed.@ -1,6 +1,8 @@"""OPML builder — generates an ``opml.xml`` index listing all channel feeds."""# SPDX-License-Identifier: AGPL-3.0-or-laterfrom datetime import UTCcode [MEDIUM]
datetime.UTC was added in Python 3.11. If this package still supports Python 3.9 or 3.10 (the type hints use list[...], not the older typing.List), this import will raise ImportError at runtime. Use from datetime import timezone and datetime.now(timezone.utc) instead for broader compatibility.
@ -27,6 +29,7 @@ def build_opml_xml(root = etree.Element("opml", attrib={"version": "2.0"})head = etree.SubElement(root, "head")etree.SubElement(head, "title").text = "Podcasts"etree.SubElement(head, "dateModified").text = datetime.now(UTC).strftime("%a, %d %b %Y %H:%M:%S +0000")code [HIGH]
strftime('%a, %d %b ...') is locale-dependent. Under a non-English locale the day and month abbreviations can be localized, producing an invalid RFC 822/OPML dateModified value. Use locale-independent formatting such as email.utils.formatdate(datetime.now(timezone.utc).timestamp(), usegmt=True) or email.utils.format_datetime, which always emits English abbreviations.
WuMing review — 1 fixed, 2 declined
src/sub2pod/opml.pyline 32✅ Fixed in commit
9db1f9d— now usesemail.utils.format_datetime(datetime.now(UTC)), which always emits English abbreviations regardless of locale.src/sub2pod/opml.pyline 4🔴 Declined —
pyproject.tomlsetsrequires-python = ">=3.12", sodatetime.UTC(added in 3.11) is always available.feed.pyalready imports it (from datetime import UTC).src/sub2pod/cli.pyline 100🔴 Declined — the warning is unreachable at the CLI level: in a normal run,
write_feed()writes every channel'sfeed.xmlbeforewrite_opml()runs, so no channel is ever missing it. A CLI-level test can't construct the missing-feed scenario. The verbose behaviour is covered by direct unit tests onwrite_opml()(warn + silent paths).WuMing
Found 4 issue(s). See inline comments below.
@ -199,6 +199,8 @@ def _append_episode_item(image_url = build_enclosure_url(base_url, channel_dir, episode.thumbnail_path)itunes_image = etree.SubElement(item_element, f"{_ITUNES}image")itunes_image.set("href", image_url)itunes_image.set("width", "3000")code [MEDIUM]
Hardcoded width/height of 3000 may not match the actual episode thumbnail. If episode.thumbnail_path is not guaranteed to be exactly 3000x3000, this produces incorrect RSS metadata. Derive the real dimensions from the image or enforce/omit these attributes.
@ -1,6 +1,9 @@"""OPML builder — generates an ``opml.xml`` index listing all channel feeds."""# SPDX-License-Identifier: AGPL-3.0-or-laterfrom datetime import UTCcode [LOW]
datetime.UTC was added in Python 3.11. If this project still supports Python 3.10 or earlier, importing opml will raise ImportError. Prefer from datetime import timezone and datetime.now(timezone.utc) for broader compatibility.
@ -27,6 +30,7 @@ def build_opml_xml(root = etree.Element("opml", attrib={"version": "2.0"})head = etree.SubElement(root, "head")etree.SubElement(head, "title").text = "Podcasts"etree.SubElement(head, "dateModified").text = format_datetime(datetime.now(UTC))code [MEDIUM]
format_datetime delegates to strftime, whose weekday/month abbreviations are locale-dependent. In a non-English locale this can emit non-RFC-822 names and make the test regex fail. Use a fixed-English formatter or force the C locale for this call.
@ -57,0 +64,4 @@if verbose:for channel_info, channel_dir in channels:if not (channel_dir / "feed.xml").exists():print(f"warning: {channel_info.title} has no feed.xml")code [LOW]
Warnings should normally go to stderr, not stdout, so normal command output remains unpolluted for scripts and pipes. Use print(..., file=sys.stderr) and update the test to capture stderr.
WuMing review — 1 escalating, 3 declined
src/sub2pod/feed.pyline 202Escalating to owner — this is a design decision (declare 3000×3000 per the issue's spec, read real dimensions via a new Pillow dependency, or omit). Will resolve separately.
src/sub2pod/opml.pyline 33🔴 Declined — incorrect.
email.utils.format_datetimeformats day/month via_format_timetuple_and_zoneusing hardcoded English name lists, deliberately avoidingstrftime(CPython source comment: "we cannot use strftime() because that honors the locale"). The onlystrftimecall is%zfor the numeric offset, which is locale-independent.src/sub2pod/opml.pyline 4🔴 Declined (re-raise of #1742) —
requires-python = ">=3.12", sodatetime.UTC(3.11+) is always available.src/sub2pod/opml.pyline 66🔴 Declined — this warning is
--verbosediagnostic output, which this codebase prints to stdout (see the existing verbose channel/episode logging incli.py). Splitting just this line to stderr would fragment the verbose stream.src/sub2pod/feed.pyline 202✅ Addressed in commit
47a3cbc— thewidth/heightattributes are now omitted entirely. The actual ytdl-sub thumbnails are 16:9 YouTube images, so declaring 3000×3000 would be inaccurate; we chose to omit rather than add an image-reading dependency.WuMing
Found 1 issue(s). See inline comments below.
@ -35,3 +35,3 @@- [ ] Update `ytdl-sub-integration.md` to reflect that per-episode thumbnails are supported- [ ] Add `width`/`height` attributes to per-episode `<itunes:image>` elements (3000×3000 recommended)- [x] Add `width`/`height` attributes to per-episode `<itunes:image>` elements (3000×3000 recommended) — decided to omit: ytdl-sub thumbnails are 16:9 YouTube images, not 3000×3000 square, so declared dimensions would be inaccuratedocs [HIGH]
This line contradicts itself and the PR description: the checkbox marks “Add width/height attributes” as completed, but the note says the change was “decided to omit.” If the attributes were not added, reword the item to reflect a decision (e.g. “[x] Decide whether to add width/height… — decided to omit…”) and update the PR description. If they were added, remove the “decided to omit” note.
improvements.mdline 37✅ Fixed in commit
411e5f5— reworded to "Decide on per-episode<itunes:image>width/height attributes — decided to omit: …". The PR description already reflects the omission.