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.

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

Allow custom UserDefaults object, bug fixed #28

Merged
merged 6 commits into from Mar 29, 2018

Conversation

albertwujj
Copy link
Contributor

@albertwujj albertwujj commented Mar 21, 2018

Added an optional parameter to Zephyr.sync(), taking a custom UserDefaults object and using that in place of always using the container-wide UserDefaults.standard.

Fixed bug resulting in UserDefaults.standard being used despite passing in a custom UserDefaults object to the variable-arguments sync() function (when no keys were passed in)

sync(keys: keys) =>
sync(userDefaults: userDefaults, keys: keys)
@ArtSabintsev
Copy link
Owner

Please cleanup all the whitespace issues. Makes it really hard to see the diff. It's probably space/tabs issue in your Xcode settings.

I will review it afterwards.

@albertwujj
Copy link
Contributor Author

albertwujj commented Mar 21, 2018

Hey, I trimmed whitespaces from blank lines with Xcode settings. Thanks for the tip!

@albertwujj albertwujj closed this Mar 21, 2018
@albertwujj albertwujj reopened this Mar 21, 2018
@albertwujj
Copy link
Contributor Author

albertwujj commented Mar 21, 2018

By the way, some lines with "///" or "/// - Parameters:" had an extra space in front of them, some didn't. I matched them anyways. Kinda satisfying.

@ArtSabintsev
Copy link
Owner

Alright, I need to think about a few things with respect to this PR, specifically with the fact that this is introducing a breaking API change and will involve some @deprecated flags.

Was there an issue creating a new sync method that added the userDefaults param, and for the default scenario, caused down to the original sync method?

@albertwujj
Copy link
Contributor Author

albertwujj commented Mar 21, 2018

for the default scenario, caused down to the original sync method?

Not sure what exactly you mean. But I didn't create any new methods. I only added an optional parameter. However, you had two sync methods, one that called the other, and I forgot to pass
the optional parameter from one method to the other.

@ArtSabintsev
Copy link
Owner

I think you should create a third sync method that calls down to the others after it does what it needs to do. Doing that makes it so the API doesn't have a breaking change.

Added 3rd and 4th overloaded sync func, with a custom UserDefaults object as an optional parameter, rather than adding the optional parameter to the original functions.
@albertwujj
Copy link
Contributor Author

Got it. I committed changes adding the 3rd and 4th overloaded sync functions (tested).

On another note, I am curious, why did you have the variadic-argument sync function deal with the empty keys case rather than have the array-argument sync function deal with both empty and non-empty cases?

@albertwujj
Copy link
Contributor Author

albertwujj commented Mar 21, 2018

To clarify though, the UserDefaults parameter was an optional parameter so, in my previous commit, one could still call the sync function as normal without passing in a UserDefaults object. The default value of the optional parameter was UserDefaults.standard.

@ArtSabintsev
Copy link
Owner

I haven't forgotten about this PR. I will look later this week.

Copy link
Owner

@ArtSabintsev ArtSabintsev left a comment

Choose a reason for hiding this comment

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

Looks good. Just make that one comment change and I'll merge this in.

Thanks!

}

/// A session-persisted variable to directly access all of the NSUbiquitousKeyValueStore elements.
private var zephyrRemoteStoreDictionary: [String: Any] {
return NSUbiquitousKeyValueStore.default.dictionaryRepresentation
}

//The UserDefaults object to sync with NSUbiquitousKeyValueStore/iCloud
Copy link
Owner

Choose a reason for hiding this comment

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

Three / and spacing afterwards.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I'll merge it in and make the change myself and release this as part of the Swift 4.1 release I'm about to cut.

Thanks!

@ArtSabintsev ArtSabintsev merged commit 1d62c5c into ArtSabintsev:master Mar 29, 2018
@ArtSabintsev
Copy link
Owner

Merged and release in v3.1.0.

Thanks again!

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.

None yet

2 participants