Refactoring the ModelChange API to use a real model instead of optionals #48
Conversation
This allows me to delete the new weak array code added here Fixing a bug
Updating docs Some style fixes
This makes the public API for this cleaner and avoids double optionals
| @@ -316,10 +316,9 @@ open class ConsistencyManager { | |||
| open func updateModel(_ model: ConsistencyManagerModel, context: Any? = nil) { | |||
| dispatchTask { cancelled in | |||
| let tuple = self.childrenAndListenersForModel(model) | |||
There was a problem hiding this comment.
tuple is not a great variable name, so I would just unwrap it on the spot.
let (modelUpdates, listeners) = self.childrenAndListenersForModel(model)
| @@ -356,7 +355,7 @@ open class ConsistencyManager { | |||
| }() | |||
|
|
|||
| // A simple update dictionary. We're just deleting a model with this id. Nothing else. | |||
| let updatesDictionary: [String: [ConsistencyManagerModel]?] = [ id: nil ] | |||
| let updatesDictionary = [ id: ModelChange.deleted ] | |||
There was a problem hiding this comment.
no space after/before [/]
let updatesDictionary = [id: ModelChange.deleted]
| // It's an update. Let's apply the changes. | ||
| let mergedReplacementModel = mergedModelFromModel(model, withUpdates: replacementModel) | ||
| let mergedReplacementModel = mergedModelFromModel(model, withUpdates: replacementModels) |
There was a problem hiding this comment.
withUpdates label seems out of date now. Should either match the type (e.g. withModelChanges) or just be with since it now has a strong type (my preference is the latter).
| */ | ||
| public enum ModelChange: Equatable { | ||
| /** | ||
| This indicates that this ID has been updated with a new model. |
There was a problem hiding this comment.
It is kind of weird to reference this ID in the documentation here, because this type knows nothing of the ID. Maybe it should?
Assuming the current implementation, I would recommend describing/documenting this type without any assumption about how it would be used.
If you want to document the [String: ModelChange] data structure used elsewhere, create a typealias (or a proper type) somewhere and document that.
There was a problem hiding this comment.
Added some docs to the delegate method which people will probably read first and changed these.
| return isEqual && tuple.0.isEqualToModel(tuple.1) | ||
| } | ||
| case (.deleted, .deleted): | ||
| return true |
There was a problem hiding this comment.
Kind of weird that all deletes are considered equal. Maybe this data structure should capture the ID or the old model for deletes.
There was a problem hiding this comment.
This is so you can do: if modelChanges[id] == .delete
I'm not sure it should capture the old ID. I think the problem here is that this captures the whole change. It doesn't, it only specifies that something has changed. For instance, updates don't capture the old model, only the new model.
| @@ -12,12 +12,26 @@ import ConsistencyManager | |||
|
|
|||
| class TestUpdatesListener: ConsistencyManagerUpdatesListener { | |||
|
|
|||
| var updateClosure: ((ConsistencyManagerModel, [String : [ConsistencyManagerModel]?], Any?) -> Void)? | |||
| var updateClosure: ((ConsistencyManagerModel, [String : ModelChange], Any?) -> ())? | |||
There was a problem hiding this comment.
Why changing this back to -> (). I thought -> Void was preferred? (3.8.2)
This makes the API much easier to use and avoids double optionals