-
Notifications
You must be signed in to change notification settings - Fork 16
Re-implement a few reader models #556
Conversation
5a981f2 to
a5805e1
Compare
mokagio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
| private let SiteDictionaryFeedIDKey = "feed_ID" | ||
| private let SiteDictionaryFeedURLKey = "feed_URL" | ||
| private let SiteDictionaryFollowingKey = "is_following" | ||
| private let SiteDictionaryJetpackKey = "is_jetpack" | ||
| private let SiteDictionaryOrganizationID = "organization_id" | ||
| private let SiteDictionaryPrivateKey = "is_private" | ||
| private let SiteDictionaryVisibleKey = "visible" | ||
| private let SiteDictionaryPostCountKey = "post_count" | ||
| private let SiteDictionaryIconPathKey = "icon.img" | ||
| private let SiteDictionaryDescriptionKey = "description" | ||
| private let SiteDictionaryIDKey = "ID" | ||
| private let SiteDictionaryNameKey = "name" | ||
| private let SiteDictionaryURLKey = "URL" | ||
| private let SiteDictionarySubscriptionsKey = "subscribers_count" | ||
| private let SiteDictionarySubscriptionKey = "subscription" | ||
| private let SiteDictionaryUnseenCountKey = "unseen_count" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered grouping these in a type?
private enum SiteDictionaryKey {
static let feedId = "feed_ID"
static let feedURL = "feed_URL"
// etc...
}
// Usage
response.number(forKey: SiteDictionaryKey.feedId)One could go a bit deeper and use enum with cases then add syntax sugar methods on NSDictionary to take away the .rawValue call, so we could write something like:
response.number(forSiteDictionaryKey: .feedId)
response.bool(forSiteDictionaryKey: .following)...but maybe that's too much overhead and we should save the time to get to a place where we can use modern Swift with Codable sooner. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a nicer Swift code for sure. I intentionally tried to keep the re-implementation "boring", or as close to the original Objective-C implementation as possible, for easier review and less risk of introducing bugs. I think, once I've translated the 40 something Objective-C types into Swift, then we can do a proper "swiftification" afterwards?
Co-authored-by: Gio Lodi <[email protected]>
Co-authored-by: Gio Lodi <[email protected]>
Description
Almost line-by-line translation of a few reader model types. One and only breaking change is the properties are changed from implicity unwrapped optional to optional.
Testing Details
Should be okay if CI is green.
versionin the.podspecfile.