nmdias / FeedKit

An RSS, Atom and JSON Feed parser written in Swift
MIT License
1.19k stars 173 forks source link

Make RSSFeedItem conform to Hashable protocol #120

Open peter40b opened 4 years ago

peter40b commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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.