search

bubelov / feedk

Web feed parser for Kotlin
GNU Affero General Public License v3.0
2 stars 0 forks source link

Parser does not support entries with permalinks only #1

Closed mormegil-cz closed 3 years ago

mormegil-cz commented 3 years ago

A valid RSS feed might use entries with no <link> and only specify <guid isPermaLink="true"> (or just <guid>, as “isPermaLink is optional, its default value is true.”), containing the link to the item. (Random feed example: https://k47.cz/rss.xml)

However, feedk currently does not support such feeds: it returns empty link, and while it returns the guid value, it does not parse or return the isPermaLink attribute.

Honestly, I see two possible solutions:

  1. When an entry has no link element and contains a <guid> permalink, the parser should just return the permalink in the link attribute (as well as in the guid element). This is simpler for the downstream users (it also causes them to support this use case without changes in their code). Also, I cannot imagine a situation where this would be a problem.
  2. Export the isPermaLink value from the parsed entries and let the downstream users cope with the two possibilities. This is more defensive, but all consumers need to modify their code to support this use case explicitly.

(Specifically, I came here because I use your news reader app which does not support such feeds currently (not only that, it crashes on them right now, see https://github.com/bubelov/news/pull/77); so I’d just like if it supported those, feel free to choose whichever way is simpler/more correct from your point of view…)

bubelov commented 3 years ago

@mormegil-cz this library is WIP based on this spec: https://www.rssboard.org/rss-specification

Looks like the spec mentions your situation here: https://www.rssboard.org/rss-specification#ltguidgtSubelementOfLtitemgt

Given that this library is only used in News app (as far as I'm aware), and it's also quite far from its stable release, I see no harm in breaking its unstable API. Option 2 would be more in line with the spec and with the goal of this library (which is to simply provide Kotlin data structures matching the underlying RSS/Atom XMLs).

Things get different in the News app scope. It's data structures are supposed to be RSS-agnostic, since it uses same structures and tables for all kinds of feeds and entries. I didn't want to invent my own structs because it would add unnecessary mental burden when it comes to readability so I borrowed well-known and documented structs from the Atom spec. So, News app will need to implement option 1 in its RSS -> Atom converter. It already does that with the other fields, so it won't require any hacks or schema changes.

bubelov commented 3 years ago

Implemented in https://github.com/bubelov/feedk/commit/6aa201e6fb0b2bc36cac3dfd6cd214de8cf4f525