-
Notifications
You must be signed in to change notification settings - Fork 33
Feature/failable uicolor init #273
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,47 @@ | ||
| import UIKit | ||
|
|
||
| public extension UIColor { | ||
| private static let divisor = CGFloat(255) | ||
|
|
||
| private typealias Components = (red: CGFloat, green: CGFloat, blue: CGFloat, alpha: CGFloat) | ||
|
|
||
| convenience init(hex: String) { | ||
| private enum ColorError: String, LocalizedError { | ||
| case invalidHexValue = "😱 Cannot convert string into `UInt64`" | ||
| case invalidHexSize = "😱 hex size not supported 😇" | ||
|
|
||
| var localizedDescription: String { rawValue } | ||
| } | ||
|
|
||
| private static let divisor = CGFloat(255) | ||
|
|
||
| convenience init(hexValue hex: String) throws { | ||
| let hex = hex.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| .replacingOccurrences(of: "#", with: "") | ||
|
|
||
| var hexValue: UInt64 = 0 | ||
|
|
||
| guard Scanner(string: hex).scanHexInt64(&hexValue) else { | ||
| fatalError("😱 Cannot convert string into `UInt64`") | ||
| throw ColorError.invalidHexValue | ||
| } | ||
|
|
||
| let components: Components = { | ||
| let components: Components = try { | ||
| switch hex.count { | ||
| case 6: return UIColor.components(fromHex6: hexValue) | ||
| case 8: return UIColor.components(fromHex8: hexValue) | ||
| default: fatalError("😱 hex size not supported 😇") | ||
| default: throw ColorError.invalidHexSize | ||
|
p4checo marked this conversation as resolved.
Outdated
|
||
| } | ||
| }() | ||
|
|
||
| self.init(red: components.red, green: components.green, blue: components.blue, alpha: components.alpha) | ||
| } | ||
|
|
||
| var hexString: String { | ||
| convenience init(hex: String) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we're making the initializer failable, we probably shouldn't keep the non-failable one. Users should probably migrate to proper try/catch, fallback to All this functionality would probably be better expressed as a macro that would convert the string to a UIColor, similar to the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure tbh. there was an ask on the KF project to change the color of a button dynamically, based on whatever Firebase value we had somewhere. so in that case we'd need a dynamic UIColor constructor. I didn't want to break backwards compatibility, which is why i thought it safe to keep the old init (which if it were up to me i'd keep the old just to not break compatibility, but happy to change if we feel like going in that direction
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. off topic: did some cleanup (at least i'd like to think so) in the last commit. can revert if too much / irrelevant. lemme know
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is no longer a need for your current project, is it still worth adding? Most use cases that I am aware rely on static values and not dynamic ones, and adding this new capability without an actual need makes me question the effort and impact on API. Furthermore, nowadays there are more modern/cleaner ways to do this, with color assets (that accept hex value), native regexes to validate values if needed, and even macros that could create and validate color values at compile time. You added some nice improvements though, so thanks for that! 🙏🏼 What are your thoughts? If you still think it's worth adding, then I think we also need to add coverage on the new functionality 🤓
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. depending on which hour of the day you ask me, i'll answer either "yes" or "no" 😅 Why yes
Why no
i added some Unit Tests for the new functionality. let me know if I:
i'm fine with either, as i'm a bit in between myself whether these changes are still relevant or not
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the sanity check. I am torn myself as well 😅 All things considerad, I think that since we already have this new functionality essentially done, as long as we don't make this a breaking change (i.e. we keep the existing We do have some issues with CI, namely on the CocoaPods validation, which need to be sorted before we can merge this though 🙈
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. apologies for late reply. got sidetracked with some project work. i didn't manage to fix the MR. looks like some sort of a fastlane problem? can't really tell. i'll be on holiday starting 11th Aug until 24th Aug. Can pick this one up when i return if needed. If you change your mind however, feel free to close it, i don't mind 😁 |
||
| do { | ||
| try self.init(hexValue: hex) | ||
| } catch { | ||
| fatalError(error.localizedDescription) | ||
| } | ||
| } | ||
|
|
||
| var hexString: String { | ||
| var components = UIColor.components(fromHex6: 0) | ||
| getRed(&components.red, green: &components.green, blue: &components.blue, alpha: &components.alpha) | ||
|
|
||
|
|
@@ -40,7 +54,6 @@ public extension UIColor { | |
| } | ||
|
|
||
| var hexStringWithAlpha: String { | ||
|
|
||
|
p4checo marked this conversation as resolved.
|
||
| var components = UIColor.components(fromHex8: 0) | ||
| getRed(&components.red, green: &components.green, blue: &components.blue, alpha: &components.alpha) | ||
|
|
||
|
|
@@ -64,7 +77,6 @@ public extension UIColor { | |
| } | ||
|
|
||
| private static func components(fromHex8 hex: UInt64) -> Components { | ||
|
|
||
| let alpha = CGFloat((hex & 0xFF000000) >> 24) / UIColor.divisor | ||
| let red = CGFloat((hex & 0x00FF0000) >> 16) / UIColor.divisor | ||
| let green = CGFloat((hex & 0x0000FF00) >> 8) / UIColor.divisor | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -501,7 +501,7 @@ extension Persistence.DiskMemoryPersistenceStack: NSCacheDelegate { | |
| } | ||
| } | ||
|
|
||
| private final class DiskMemoryBlockOperation: BlockOperation { | ||
| private final class DiskMemoryBlockOperation: BlockOperation, @unchecked Sendable { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove concurrency changes as we haven't migrated the project to using Swift Concurrency yet
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i reverted this but now on my machine the project doesn't compile. is it because Xcode 16 or smth?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it's probably because you're building it in Swift 6 mode or swft 5.10 with full concurrency checking |
||
|
|
||
| required init(qos: QualityOfService = .default, block: @escaping () -> Swift.Void) { | ||
| super.init() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.