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

Make RSSFeedItem conform to Hashable protocol #120

Open
peter40b opened this issue May 3, 2020 · 4 comments
Open

Make RSSFeedItem conform to Hashable protocol #120

peter40b opened this issue May 3, 2020 · 4 comments

Comments

@peter40b
Copy link

peter40b commented May 3, 2020

Thanks for this great cocoapod.

I came across a need to check for duplicate RSSFeedItems. It is really convenient to convert the array of items into a Set and then back to array, however, at a minimum, the RSSFeedItem class needs to conform to Hashable protocol. I wrote this tiny extension to RSSFeedItem.swift. Note, there may be other classes that could also benefit from this feature, but this solved my immediate problem.


extension RSSFeedItem: Hashable {
    
    public func hash(into hasher: inout Hasher) {
        hasher.combine(pubDate)
        hasher.combine(author)
        hasher.combine(description)
        hasher.combine(title)
    }
}
@ghost
Copy link

ghost commented May 3, 2020

Hi,

Thank you for sharing. Has I see it, hashable should check the guid or link element, as per specification, if I recall correctly. I’m glad the solution works for you, but it makes too many assumptions for everyone else. I could be convinced otherwise of course 😸 also, depending on the description element for this.. yikes feels a bit hardcore to integrate into FeedKit

Thanks 🙏

@GarthSnyder
Copy link
Collaborator

@tarjamorgado, this seems like a reasonable standard, but would it not apply equally well to ==, which RSSFeedItem already defines? It looks like the horse may already be out of the barn since == doesn't include link equality:

extension RSSFeedItem: Equatable {
    public static func ==(lhs: RSSFeedItem, rhs: RSSFeedItem) -> Bool {
        return
            lhs.author == rhs.author &&
            lhs.categories == rhs.categories &&
            lhs.comments == rhs.comments &&
            lhs.content == rhs.content &&
            lhs.description == rhs.description &&
            lhs.dublinCore == rhs.dublinCore &&
            lhs.enclosure == rhs.enclosure &&
            lhs.guid == rhs.guid &&
            lhs.iTunes == rhs.iTunes &&
            lhs.media == rhs.media &&
            lhs.pubDate == rhs.pubDate &&
            lhs.source == rhs.source &&
            lhs.title == rhs.title
    }
}

From a practical standpoint, any reasonably unique subset of the fields used by == would make sense as hash contributors. It does make sense to define a standard hash; otherwise you're just forcing users to read the source code to make sure their custom hash functions are compatible with ==.

For what it's worth, it seems to be common for feeds to include multiple items with the same link, and most feeds don't include proper GUIDs. My experience is limited to podcast feeds, but I wouldn't be surprised if other media ecosystems show these same deficiencies.

@ghost
Copy link

ghost commented May 4, 2020

@GarthSnyder you make a good point 🙏 and looking at that == operator to make it conform to Equatable reminds me there was a swift proposal that was implemented to auto synthesize Structs for Equatable and apparently also Hashable!

I'm trying to see if there is any good reason not to migrate all theses classes into structs, and gain Equatable and Hashable conformance "for free" 🤔

@peter40b
Copy link
Author

peter40b commented May 5, 2020

@tarjamorgado I like the struct idea.

@GarthSnyder Well, my first reaction was to add everything in the == to the hash, but then since many of those classes weren't conforming either (and would involve a cascading series of Hashable extensions (yuck), I thought it easiest to trim back my expectations to tune just the one class. And, my goal was to present only one instance of a RSSFeedItem, precisely because I was seeing dupes and wanted to avoid having to explain the behavior in the UI.

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

No branches or pull requests

2 participants