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

Add RealmConfiguration / RLMConfiguration #2326

Merged
merged 21 commits into from
Aug 13, 2015
Merged

Add RealmConfiguration / RLMConfiguration #2326

merged 21 commits into from
Aug 13, 2015

Conversation

segiddins
Copy link
Contributor

Closes #1584

@segiddins segiddins added the pr label Aug 1, 2015
@end

/**
Migration block used to migrate a Realm.
Copy link
Contributor

Choose a reason for hiding this comment

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

Configuration not migration.

@jpsim
Copy link
Contributor

jpsim commented Aug 1, 2015

This is moving in a great direction! Well done. Although I'd much prefer we deprecate the older APIs now rather than later. This will need tests and a few changelog entries too.

@kishikawakatsumi
Copy link
Contributor

Would you like to support NSFileProtection attributes and NSURLIsExcludedFromBackupKey?

@jpsim
Copy link
Contributor

jpsim commented Aug 1, 2015

@kishikawakatsumi this PR is just a first step meant to move our current API and functionality to this new interface. Adding new functionality is out of scope for this PR, but should be pretty simple to add afterwards.

@segiddins
Copy link
Contributor Author

@jpsim I explicitly didn't deprecate the existing APIs because the tests rely on them so heavily

@segiddins
Copy link
Contributor Author

@jpsim would you also like me to deprecate the existing apis for setting encryption keys and schema versions per path?

@jpsim
Copy link
Contributor

jpsim commented Aug 1, 2015

would you also like me to deprecate the existing apis for setting encryption keys and schema versions per path?

Yes, it seems logical to me to minimize the number of ways to do the same thing, and I believe we're better off doing so sooner rather than later.

@segiddins
Copy link
Contributor Author

OK. Marking those as deprecated (and cleaning up the tests) will take a bit of work.

if (!realm.notifier) {
return nil;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

whitespace

@tgoyne
Copy link
Member

tgoyne commented Aug 3, 2015

I don't understand what the goal of RLMConfigurator is. It's not a pattern that our users will be familiar with, and AFAICT the only benefit we get from is having to make a copy of the configuration object when setting the default one. If we do actually need an immutable version, RLMConfiguration/RLMMutableConfiguration would be much more normal and would be a similar amount of code both in the implementation and the code using it.

@segiddins
Copy link
Contributor Author

@tgoyne the reason I went with that pattern is that it's the closest way possible of mimicking structs in Objective-C

@tgoyne
Copy link
Member

tgoyne commented Aug 3, 2015

The equivalent of a struct in obj-c is a class with mutable data members that you explicitly copy rather than implicitly copy.

@segiddins
Copy link
Contributor Author

OK, changed to just be mutable. I'll add some tests if this design looks better?

An `RLMConfiguration` is used to describe the different options used to
create an `RLMRealm` instance.
*/
@interface RLMConfiguration : NSObject<NSCopying>
Copy link
Member

Choose a reason for hiding this comment

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

There's a few things here that talk about the "default configuration", but nothing ever says what that is or what it's used for.

Realm needs to be capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@segiddins
Copy link
Contributor Author

I believe I've now addressed all of the outstanding comments?

@segiddins
Copy link
Contributor Author

Would like to get this merged soon, since I've started work on adding some features based on this PR

@segiddins
Copy link
Contributor Author

@bdash could you please remind me what you see as the inconsistency?

@bdash
Copy link
Contributor

bdash commented Aug 12, 2015

This is still relevant after the move from RealmConfiguration to Realm.Configuration:

You haven't elaborated on why Configuration is too generic in the Swift API only, nor on why it is special in that regard. Is Configuration too generic because it's possible we'll want to make some other object in RealmSwift configurable in the future? If so, why wouldn't we also give the Objective-C class a name that indicates that it is a configuration for a Realm (e.g., RLMRealmConfiguration). Is there something I'm missing that makes the need to qualify the typename specific to the Swift API? My impression is that either both should have the extra Realm qualification, or neither should.

@segiddins
Copy link
Contributor Author

Realm.Configuration is not nearly as generic as just Configuration -- the type shows that it's something that configures Realm (as a framework). The RLM prefix also signifies this.

@bdash
Copy link
Contributor

bdash commented Aug 12, 2015

Being a generic term doesn't seem like a strong argument for special treatment given that we declare types with names like Object and List. Why is Configuration different from those other types?

Your description of the Realm prefix sounds like it's duplicating the namespacing that's already provided by Swift's modules.

@segiddins
Copy link
Contributor Author

I personally dislike that we have things called Object and List, but at least they're incredibly common parts of our API that will basically be used wherever Realm is used in a user's app. The configuration will probably only be created once, and it is an ancillary, supporting class, not something crucial to using Realm.

@tgoyne
Copy link
Member

tgoyne commented Aug 12, 2015

I suggested Realm.Configuration on the grounds that it exists only to be passed to Realm's initializer (and setDefaultRealm), and so it feels natural to reduce its scope as much as possible and nest it within Realm. We can't do the same thing in obj-c, but I wouldn't be opposed to RLMRealmConfiguration.

@segiddins
Copy link
Contributor Author

Renamed.

@bdash
Copy link
Contributor

bdash commented Aug 12, 2015

👍

@segiddins
Copy link
Contributor Author

CHANGELOG added

@segiddins
Copy link
Contributor Author

Test failures all look unrelated (open() failed or testNotificationForChangesWhileSuspended)

segiddins added a commit that referenced this pull request Aug 13, 2015
Add RealmConfiguration / RLMConfiguration
@segiddins segiddins merged commit 6e87d00 into master Aug 13, 2015
@segiddins segiddins removed the pr label Aug 13, 2015
@segiddins segiddins deleted the seg-configuration branch August 13, 2015 04:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RLMConfiguration
5 participants