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.

Sign up for GitHub

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

Use Gravatar.ImageCaching protocol instead of defining our own #146

Merged
merged 2 commits into from
Feb 26, 2024
Merged

Use Gravatar.ImageCaching protocol instead of defining our own #146

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b06e5c9
Use Gravatar.ImageCaching protocol instead of defining our own
andrewdmontgomery Feb 16, 2024
fc0e664
Re-export the `ImageCaching` protocol from Gravatar module
andrewdmontgomery Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions Sources/WordPressUI/Extensions/UIImageView+Networking.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Foundation
import UIKit
import protocol Gravatar.GravatarImageCaching
@_exported import protocol Gravatar.ImageCaching

#if SWIFT_PACKAGE
import WordPressUIObjC
Expand Down Expand Up @@ -173,11 +173,6 @@ public extension UIImageView {
}
}

public protocol ImageCaching: GravatarImageCaching {
func setImage(_ image: UIImage, forKey key: String)
func getImage(forKey key: String) -> UIImage?
}

Comment on lines -176 to -180
Copy link
Author

Choose a reason for hiding this comment

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

Is there a reason we were creating our own protocol in WPUI that conforms to the Gravatar protocol? After the renaming, this got tricky. And it seemed to make sense to drop this in favor of using the Gravatar.ImageCaching protocol.

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably existed before Gravatar SDK, and now we have the same name for the same definition of protocol.
I agree, we can drop it unless it's used somewhere else as argument independently from Gravatar, which doesn't seem to be the case 🤔

Copy link
Contributor

@pinarol pinarol Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason we were creating our own protocol in WPUI that conforms to the Gravatar protocol?

We didn't. This was here already.

Unfortunately we can't just drop a public type from an open source library. We can't easily know where it's been used. We should always deprecate first even if we want to remove it in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we can't just drop a public type from an open source library

Oh that's right!

One alternative, if we don't want to re-define the procotol, we can do this:

@_exported import protocol Gravatar.ImageCaching

With @_exported, WPUI will re-export ImageCaching as public, and the API won't change.
Making the changes on WPiOS not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! This can work!

Copy link
Author

Choose a reason for hiding this comment

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

Oh nice! Didn't know about this. Perfection. I'll push this. Thanks!

public class ImageCache: ImageCaching {
private let cache = NSCache<NSString, UIImage>()

Expand Down