uber-archive / ohana-ios

Contacts simplified. This project is deprecated and not maintained.
MIT License
362 stars 40 forks source link

DataSource API consolidation proposal #31

Closed adam-zethraeus closed 8 years ago

adam-zethraeus commented 8 years ago

The relevant part of the DataSource's current API looks like this:

/**
 *  Signal fired after the data source has finished loading all of its data providers
 */
@property (nonatomic, readonly) UBEmptySignal *onContactsDataSourceLoadedProvidersSignal;

/**
 *  Signal fired after the data source has finished running all of its post processors
 */
@property (nonatomic, readonly) UBEmptySignal *onContactsDataSourcePostProcessorsFinishedSignal;

/**
 *  Signal fired after the data source is ready to be used
 *
 *  @discussion When this signal is fired, the `contacts` property will have been set
 */
@property (nonatomic, readonly) UBEmptySignal *onContactsDataSourceReadySignal;

I propose removing the onContactsDataSourceLoadedProvidersSignal and onContactsDataSourcePostProcessorsFinishedSignal from the public API. They're unused externally in the example app and are really implementation details.

I also propose making the onContactsDataSourceReadySignal actually fire a list of of contacts.

Any questions or concerns?

adam-zethraeus commented 8 years ago

additionally, the exposed readonly data providers and post processors have little value. I'd like to remove these fields.

/**
 *  Ordered set of data providers that the data source was initialized with
 */
@property (nonatomic, readonly) NSOrderedSet<id<OHContactsDataProviderProtocol>> *dataProviders;

/**
 *  Ordered set of post processors that the data source was initialized with
 */
@property (nonatomic, readonly, nullable) NSOrderedSet<id<OHContactsPostProcessorProtocol>> *postProcessors;
maxwellE commented 8 years ago

I disagree on the second post here, having access to the data providers is needed, post processors not so much

maxwellE commented 8 years ago

I also propose making the onContactsDataSourceReadySignal actually fire a list of of contacts.

Sounds great to me!

maxwellE commented 8 years ago

Might make more sense to move some of these headers into an extensions category that users can choose to pull in or not. That way we Don't have to delete them but they are not part of the top level API

adam-zethraeus commented 8 years ago

@maxwellE can you give an example where it's useful to expose the DataProviders here? I might be being daft but i'm not seeing the value in general, and more specifically i'm not understanding why the DataSource should be the one doing it. (If a consumer truly needs a DataProvider later, can they just hold it?)

adam-zethraeus commented 8 years ago

Also i'm heavily pro making the interface lean for understandability's sake, and if the properties are superfluous, moving them to a category doesn't change that.

If they're not superfluous we should, of course, keep them somehow.