wvteijlingen / Spine

A Swift library for working with JSON:API APIs. It supports mapping to custom model classes, fetching, advanced querying, linking and persisting.
MIT License
266 stars 109 forks source link

Consider making `addResourceAsExisting` public func #56

Closed krasnoukhov closed 8 years ago

krasnoukhov commented 8 years ago

In my project, I have a resource that has toMany relation with finite list of other resources (ids are known as well).

It would be nice to be able to have this func public as well as addResourcesAsExisting since it helps me to manually assign resources that I know already persisted in order to properly track further changes and persist them later.

Let me know what you think and if you want me to make a change.

wvteijlingen commented 8 years ago

Just for clarity, those resources you want to add are already persisted and already linked to the parent resource? If so, then yes, this sounds useful to have in public. I'm thinking about renaming the methods as follows to make a clearer distinction between linking/unlinking vs just adding to the local array:

addResource -> linkResource
addResources -> linkResources
removeResource -> unlinkResource
removeResources -> unlinkResources

addResourceAsExisting -> appendResource
addResourcesAsExisting -> appendResources

The append methods are probably more suited to be put into the regular ResourceCollection, since they have nothing to do with linking/unlinking. Also, append instead of add to be more in line with native Swift array naming.

wvteijlingen commented 8 years ago

Also, perhaps we could add some assertions to make sure you cannot link/unlink/append resources that haven't been persisted. In practice you can now, but they won't be persisted or linked when the parent resource is saved. This might be confusing and perhaps it's better to remove the ability to do this altogether.

wvteijlingen commented 8 years ago

https://github.com/wvteijlingen/Spine/tree/collection-mutators

krasnoukhov commented 8 years ago

Just for clarity, those resources you want to add are already persisted and already linked to the parent resource?

Yes, that is correct. Proposed changes totally make sense to me, thanks!

wvteijlingen commented 8 years ago

Done! https://github.com/wvteijlingen/Spine/pull/58