Skip to content

Conversation

@spydercapriani
Copy link

Looks to address this issue:
#251

@sunshinejr-bot
Copy link

sunshinejr-bot commented Dec 2, 2020

Warnings
⚠️ Any changes to library code should be reflected in the Changelog.

Generated by 🚫 Danger Swift against 0f37d9d

Copy link
Owner

@sunshinejr sunshinejr left a comment

Choose a reason for hiding this comment

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

Hey @spydercapriani - thanks for the PR! 🙏 Few questions left in comments, please let me know if it makes sense!

// 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)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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 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 = []) {
Copy link
Owner

Choose a reason for hiding this comment

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

so ideally I'd love to have the Defaults be a default adapter - can we do that?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

@spydercapriani yeah that would work - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants