Skip to content

Add better, more flexible APIs for SDK initialization. #570

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

Merged
merged 1 commit into from
Dec 19, 2015

Conversation

richardjrossiii
Copy link
Contributor

This PR includes public API Changes

Old initialization looked somewhat outdated, our new APIs allow a usage similar to this:

ObjC

[Parse initializeWithConfiguration:[ParseClientConfiguration configurationWithBlock:^(ParseClientConfiguration *configuration) {
    configuration.applicationId = ...;
    configuration.clientKey = ...;
    configuration.localDatastoreEnabled = ... ;

    // Data sharing config, etc.
}];

Swift

Parse.initializeWithConfiguration(ParseClientConfiguration() {
    $0.applicationId = ""
    $0.clientKey = ""
    $0.localDatastoreEnabled = true
    // etc.
})

We believe that this provides a much cleaner interface for initialization. Please feel free to comment below on any feedback that you may have on this


Our current SDK initialization is lackluster in that it's split across multiple classes, files, and method calls.

This PR hopes to unify some of these initialization settings int o a single 'configuration' class that can be easily used to intialize parse in a much cleaner way, especially as we add even more initialization options.

*/

#import <Parse/PFConstants.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

Nullability annotate, pretty please. 😁

@property (nonatomic, copy) NSString *applicationId;
@property (nonatomic, copy) NSString *clientKey;

@property (nonatomic, assign, getter=isLocalDatastoreEnabled)BOOL localDatastoreEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Space before BOOL

@richardjrossiii richardjrossiii force-pushed the richardross.initialization.update branch 2 times, most recently from 1b588bf to 2497c8b Compare November 16, 2015 22:35
@interface ParseClientConfiguration : NSObject<NSCopying>

+ (instancetype)new NS_UNAVAILABLE;
+ (instancetype)alloc NS_UNAVAILABLE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need these, simply due to the fact that you can't init it anyway after allocating... 2 less methods polluting the name space?

@richardjrossiii
Copy link
Contributor Author

Hey all, just added some preliminary APIs for configuring URL sessions & retry attempts that we use in the SDK. Any feedback would be great!

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

@oferRounds
Copy link

Hi @richardjrossiii
Thanks for your very detailed and answer and for your very quick respond!
The changes you've added look great! That will suit exactly what I'm looking for.

When do you think this PR could be merged into master?

@@ -67,6 +68,22 @@ NS_ASSUME_NONNULL_BEGIN
+ (void)setApplicationId:(NSString *)applicationId clientKey:(NSString *)clientKey;

Choose a reason for hiding this comment

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

I never realized that iOS uses set instead of initialize. I do like moving towards initialize since it's not exactly a setter, but is it weird to have it be named differently?

Also, if we keep initialize, we should probably update the abstract of it from Sets the configuration to be used for parse to Initialize Parse with a configuration or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My best guess for the reason that we use set instead of initialize on iOS - +initialize is a method declared by NSObject, equivalent to a static contractor in java-land.

It would have caused some ambiguity for developers if we had a method call like [Parse initialize:@"appKey" clientKey:@"ck"], but we can get around that now by marking +initialize as unavailable for use (which wasn't possible when Parse was first created).

Choose a reason for hiding this comment

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

Ah that makes sense, SGTM!

@grantland
Copy link

Somewhat nitpicky, but ParseClientConfiguration seems super long. Would ParseConfiguration be confusing with ParseConfig?

@nlutsenko
Copy link
Contributor

Somewhat nitpicky, but ParseClientConfiguration seems super long. Would ParseConfiguration be confusing with ParseConfig?

Yup, it's definitely confusing. It's ObjC anyway, so long names are expected...
Plus - this way lets us name the class in the same way across multiple platforms (there is ParseConfig already on the Java).

Another potential name that could work is ParseClientConfig, but then it's still somewhat confusing compared to ParseConfig.

#pragma mark - Init
///--------------------------------------

- (instancetype)initWithConfigurationBlock:(void (^)(id<ParseMutableClientConfiguration>))configurationBlock {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to initWithBlock:, as you don't have it in the static constructor.

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

_applicationGroupIdentifier = [applicationGroupIdentifier copy];
_containingApplicationIdentifier = [containingApplicationIdentifier copy];
///--------------------------------------
#pragma mark - Offline Store
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the appropriate place for it, as startManaging doesn't quite relate to Offline Store only.

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

}

- (instancetype)initWithBlock:(void (^)(id<ParseMutableClientConfiguration>))configurationBlock {
self = [super init];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe invoke -initEmpty here and declare that as internal designated initializer (class extension in the .m file). Don't feel strongly about it, tho.

@nlutsenko
Copy link
Contributor

One small nit, otherwise - it's perfect!

@richardjrossiii richardjrossiii force-pushed the richardross.initialization.update branch from 93bf830 to 61c2f8d Compare December 18, 2015 23:23
@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

Our current SDK initialization is lackluster in that it's split across multiple classes, files, and method calls.

This PR hopes to unify some of these initialization settings int o a single 'configuration' class that can be easily used to intialize parse in a much cleaner way, especially as we add even more initialization options.
@richardjrossiii richardjrossiii force-pushed the richardross.initialization.update branch from 61c2f8d to f43a96e Compare December 18, 2015 23:27
richardjrossiii added a commit that referenced this pull request Dec 19, 2015
….update

Add better, more flexible APIs for SDK initialization.
@richardjrossiii richardjrossiii merged commit 5d633fc into master Dec 19, 2015
@richardjrossiii richardjrossiii deleted the richardross.initialization.update branch December 19, 2015 00:02
@oferRounds
Copy link

Hi @richardjrossiii
Sorry for the question, I'm just not completely sure - if I update my Parse SDK using CocoaPods, will this branch be included?

@nlutsenko
Copy link
Contributor

@oferRounds No, not yet, this is going to be included in 1.12.0 release (per milestone on the Pull Request).

@oferRounds
Copy link

@richardjrossiii - cool, thanks!

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

@parse-github-assistant
Copy link

The label type:feature cannot be used here.

@parse-github-assistant parse-github-assistant bot removed the type:feature New feature or improvement of existing feature label Dec 6, 2021
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.

6 participants