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 empty name during import and use FeedDiscovery to try to figure the name out if possible #1266

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

renatolond
Copy link

While testing stringer locally, I tried to import my feeds from The Old Reader. The OPML generated by The Old Reader doesn't contain the feed names, so the import failed.
I made a change to allow nils and the import went through, but then the feeds had no name and it was hard to manage the feeds.

So I replicated the logic from Feed::Create to try to get a proper title for the Feed.

This will make the import slower in the case there's nameless entries, but the result seems to be better. I'm falling back to the url if the call to the feed failed to avoid retrying on dead feeds.

@renatolond renatolond force-pushed the import_name_if_not_available branch from 6e109d3 to 84fe176 Compare January 8, 2025 17:15
@mockdeep mockdeep self-assigned this Jan 8, 2025
Copy link
Collaborator

@mockdeep mockdeep left a comment

Choose a reason for hiding this comment

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

A couple of minor comments. Thanks for contributing!

feed.last_fetched = 1.day.ago if feed.new_record?
feed.group_id = group.id if group
feed.save
end

def find_feed_name(feed, parsed_feed)
return unless parsed_feed[:name].nil? && feed.name.nil?
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like these will be the same being that we're querying for feed based on the name from parsed_feed. Could we instead do:

Suggested change
return unless parsed_feed[:name].nil? && feed.name.nil?
return if feed.name?

Copy link
Author

@renatolond renatolond Jan 10, 2025

Choose a reason for hiding this comment

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

It looks like it, but the parsed_feed.slice(:name, :url) in L31 is a bit misleading. If there's only one of the fields, it will only return that one:

3.4.1 :001 > parsed_feed = { name: "foo", url: "http://foo.com" }
 => {name: "foo", url: "http://foo.com"} 
3.4.1 :002 > parsed_feed.slice(:name, :url)
 => {name: "foo", url: "http://foo.com"} 
3.4.1 :003 > parsed_feed = { name: "foo" }
 => {name: "foo"} 
3.4.1 :004 > parsed_feed.slice(:name, :url)
 => {name: "foo"} 
3.4.1 :005 > parsed_feed = { url: "http://foo.com" }
 => {url: "http://foo.com"} 
3.4.1 :006 > parsed_feed.slice(:name, :url)
 => {url: "http://foo.com"} 

In other words, slice will not return nil fields, fields not found are just ignored.

I'll tweak the spec to make that clearer, I thought about it but forgot to do it

app/commands/feed/import_from_opml.rb Outdated Show resolved Hide resolved
@mockdeep mockdeep assigned renatolond and unassigned mockdeep Jan 9, 2025
Co-authored-by: Robert Fletcher <[email protected]>
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