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

RssAgent: Migrate from FeedNormalizer to Feedjira #1564

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Conversation

knu
Copy link
Member

@knu knu commented Jun 28, 2016

This replaces #957.

ENCLOSURE_ATTRS = %i[url type length]

unless dependencies_missing?
class AtomAuthor
Copy link
Member

Choose a reason for hiding this comment

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

Should we pull this into a separate file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. Would app/models/agents/rss_agent/feedjira_extension.rb be a good place?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe in lib/feedjira_extension.rb?

@cantino
Copy link
Member

cantino commented Jun 28, 2016

Awesome! Should we ask some of the people from #889 and #955 to test this?

@@ -139,21 +160,80 @@ def check_and_track(entry_id)
end
end

unless dependencies_missing?
require ''
Copy link
Member

Choose a reason for hiding this comment

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

require ''?

@knu
Copy link
Member Author

knu commented Jul 2, 2016

Oh, I didn't mean to push this yet. I'll fix it later.

@cantino
Copy link
Member

cantino commented Jul 3, 2016

Gotcha.

@knu knu force-pushed the rss_agent-feedjira branch 2 times, most recently from d84faf8 to f5185c1 Compare July 28, 2016 09:50
@knu
Copy link
Member Author

knu commented Jul 28, 2016

OK, I think this is finally getting ready for merge. Since I've refactored and rewritten a large portion of what I pushed the last time, I've squashed everything into a single commit so the whole change gets reviewed again from the ground up. Thank you for your review and testing!

@@ -53,18 +52,43 @@ def default_options
Events look like:

{
"feed": {
Copy link
Member

@cantino cantino Jul 28, 2016

Choose a reason for hiding this comment

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

Maybe

{
  "feed": {  # if you set include_feed_info to true 

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually there was a note at the bottom but it was probably unnoticeable. I've added a couple of additional notes about some fields, so it has hopefully gained its presence.

@cantino
Copy link
Member

cantino commented Jul 28, 2016

This is a significant piece of work, thank you!

Want to ask folks in #889 and #955 to test this?

@knu
Copy link
Member Author

knu commented Jul 28, 2016

@cantino Yes, I'd much appreciate if people who have had trouble with the RssAgent could test this out and give some feedback. /cc @marycanady, @schulzpin, @Jngai

@knu
Copy link
Member Author

knu commented Aug 11, 2016

I've been running this with various feeds for two weeks without a problem. I'm afraid I was too late to come up when people had already found out their way to work around their problems, but is there any feedback from you folks?

I'd like to merge this soon, to move away from obsolete libraries and add features based on the new code base.

@cantino cantino mentioned this pull request Aug 15, 2016
@knu
Copy link
Member Author

knu commented Oct 5, 2016

@cantino Is it about time we should merge this and move on?

@cantino
Copy link
Member

cantino commented Oct 5, 2016

Yes @knu, I agree. Go for it!

FeedNormalizer is no longer maintained, and its Atom support has flaws
in that it throws away what RSS::Parser returns and falls back to using
SimpleRSS which is not capable of handling XML entities, resulting in
getting ususable URLs such as ones including `&`.

Feedjira is highly customizable as it implements parsers for various
feed formats on its own using sax-machine.

The `clean` option is reimplemented using the loofah gem.
`Feedjira::FeedUtilities#sanitize_entries!` is not used because it
tries to sanitize non-HTML properties too.

A new option `include_feed_info` is added, with which turned on feed
information is added to each event payload.

A new key `links` is added, which lists all `link` elements.

A new key `enclosure` is added.
@knu knu merged commit 5596dde into master Oct 6, 2016
@knu
Copy link
Member Author

knu commented Oct 6, 2016

Done!

@knu knu deleted the rss_agent-feedjira branch October 6, 2016 03:07
@dsander
Copy link
Collaborator

dsander commented Oct 6, 2016

Awesome work 👏

@knu knu restored the rss_agent-feedjira branch October 7, 2016 10:31
@knu
Copy link
Member Author

knu commented Oct 7, 2016

Thanks. Google Reader is long gone and it's been one year since Yahoo Pipes shut down, but feeds shall not die!

@mcanady
Copy link

mcanady commented Oct 7, 2016

Thanks for your work on this--I'm in "set it and forget it" mode for this project so I can't test now, but will let you know when I do!

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.

None yet

4 participants