Fixes PropertyWrapper #267
Conversation
Generated by 🚫 Danger Swift against 0f37d9d |
There was a problem hiding this comment.
Hey @spydercapriani - thanks for the PR! 🙏 Few questions left in comments, please let me know if it makes sense!
| @@ -91,6 +91,7 @@ public extension UserDefaults { | |||
| // swiftlint:disable:next force_cast | |||
| return _value as! T | |||
| } else if let defaultValue = key.defaultValue { | |||
| T._defaults.save(key: key._key, value: defaultValue, userDefaults: self) | |||
There was a problem hiding this comment.
can we remove the default behavior change from this PR and create another one for it? the reason is that this is a change that breaks compatibility with the current implementation, and so we would need a major version for that vs we don't have for the property wrapper fixes
There was a problem hiding this comment.
Sorry, this was suppose to be an internal change specific to our use case and not something for this PR. Didn't realize this PR was opened against our Master branch, will fix.
| } | ||
| } | ||
|
|
||
| private var _value: T.T? | ||
| private var observation: DefaultsDisposable? | ||
|
|
||
| public init<KeyStore>(keyPath: KeyPath<KeyStore, DefaultsKey<T>>, adapter: DefaultsAdapter<KeyStore>, options: SwiftyUserDefaultOptions = []) { | ||
| public init(keyPath: KeyPath<KeyStore, DefaultsKey<T>>, adapter: DefaultsAdapter<KeyStore>, options: SwiftyUserDefaultOptions = []) { |
There was a problem hiding this comment.
so ideally I'd love to have the Defaults be a default adapter - can we do that?
There was a problem hiding this comment.
So I was thinking we would be able to utilize a global framework property as a default initializer but that doesn't appear to be the case. I might suggest that we set default initializer with the same adapter init used for Default for this public init. This implies that unless a different adapter is provided, it will always look for key/values in the standard UserDefaults using the DefaultsKeyStore. Which may be practical for most basic use cases.
There was a problem hiding this comment.
@spydercapriani yeah that would work - thanks!
Looks to address this issue:
#251