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

COObject API to make it easier to use/discover COCopier #47

Open
qmathe opened this issue Aug 28, 2016 · 5 comments
Open

COObject API to make it easier to use/discover COCopier #47

qmathe opened this issue Aug 28, 2016 · 5 comments

Comments

@qmathe
Copy link
Member

qmathe commented Aug 28, 2016

In EtoileUI, the base persistent class (ETUIObject) implements the following:

- (id) copyToObjectGraphContext: (COObjectGraphContext *)aDestination
{
    NILARG_EXCEPTION_TEST(aDestination);
    ETUUID *newItemUUID = [[COCopier new] copyItemWithUUID: [self UUID]
                                                              fromGraph: [self objectGraphContext]
                                                                toGraph: aDestination];

    return [[self objectGraphContext] loadedObjectForUUID: newItemUUID];
}

- (id) copyWithZone: (NSZone *)aZone
{
    return [self copyToObjectGraphContext: [self objectGraphContext]];
}

Not really convinced we should do it, but it could be interesting to have -copyToObjectGraphContext: moved up into COObject.

If we don't change the API, we should probably have a line in COObject class description pointing to COCopier doc where we show how to leverage COCopier and implement copying logic like -copyToObjectGraphContext:.

@qmathe
Copy link
Member Author

qmathe commented Oct 4, 2016

Any comments about this suggestion?

Should I close it or keep it around?

@ericwa
Copy link
Member

ericwa commented Oct 4, 2016

IIRC, the main reason I wanted COCopier to be a separate class was to enforce that it's not accessing private COObject state or doing any magic. I think there is some value in this for the API user as well (you can tell it's implemented using public API, or at least it's likely).

What about just adding those convenience methods to COCopier?

- (COObject *)copiedObject: (COObject *)anObj
toObjectGraph: (COObjectGraphContext *)dest;

- (COObject *)copiedObject: (COObject *)anObj;

I'm fine with them in COObject too, but I feel this is a bit better (less object oriented, more modular)

@qmathe
Copy link
Member Author

qmathe commented Oct 7, 2016

Sounds good to me. In addition, I would probably move COCopier to StorageDataModel along COItem, since COCopier doesn't really belong to the high-level API. It looks like we keep it in Core to make it discoverable.

If we move it to StorageDataModel, then we could add the methods you suggest in a category like COCopier+Core or COCopier+Object. If we go with this solution, should we put this category into COObject.h/m files or into standalone files?

I'm in favor of tweaking the methods naming a bit too:

 // the context part would be discarded when renaming to COObjectGraphContext to COObjectGraph
-copyObject:toObjectGraphContext:
-copyObject:

@ericwa
Copy link
Member

ericwa commented Oct 7, 2016

Yep, agree on all points.. the COObject copying method could go in COCopier+Object

@qmathe
Copy link
Member Author

qmathe commented Oct 7, 2016

ok, let do this then :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants