Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow the use of atom rss feeds #119

Closed
wants to merge 1 commit into from
Closed

Conversation

cogfor
Copy link

@cogfor cogfor commented Aug 26, 2019

Currently, News-please does not parse atom feeds. This PR fixes that.

@fhamborg fhamborg self-assigned this Aug 27, 2019
@fhamborg fhamborg self-requested a review August 27, 2019 07:56
@fhamborg
Copy link
Owner

fhamborg commented Aug 27, 2019

That's nice. However, I currently don't think I see all the implications of this change, e.g., if later parts in the internal workflow would also work, since this PR only changes the filter criterion. There are also a few other things to consider, e.g., quite obvious that the variable names do not match anymore, e.g., the variable "re_rss" should be named "re_stream" (or something like that), since rss is not a subset of atom or vice versa. Did you test the full workflow?

Edit: Looks like there are some technical differences in the formats (rss vs atom) that require changes in later parsing of the stream, see https://www.quora.com/What-is-the-difference-between-rss-feed-and-atom-feed

@JeromeGill
Copy link
Contributor

@fhamborg would you prefer a PR with atom support in a separate crawler (leaving the user to have to understand what type of feed they are consuming) or would you prefer "RssCrawler" refactored to handle both?

@fhamborg
Copy link
Owner

Hi Jerome! Thanks for helping with news-please :-) I guess it depends on the amount of similarities between both "procedures", e.g., if it was only a few lines being different, it would be fine to have a single class handling both RSS and Atom. The class should be called FeedCrawler maybe instead of RssCrawler. But if RSS and Atom actually required significantly different processing, then we should have two different classes.

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.

3 participants