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

[WIP] Moved JSON dictionary and array transformers to MTLJSONAdapter #258

Closed

Conversation

dcaunt
Copy link
Member

@dcaunt dcaunt commented Mar 6, 2014

Implements changes suggested in #203

Closes #203

// class must conform to <MTLJSONSerializing>. This argument must
// not be nil.
//
// Returns a reversible transformer which uses MTLJSONAdapter for transforming
Copy link
Member

Choose a reason for hiding this comment

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

This should point out that we always use the class of the receiver.

@robb
Copy link
Member

robb commented Mar 6, 2014

👍 nice!

Can you add a test that verifies that if this is invoked on a subclass of MTLJSONAdapter, the subclass is used for serialization?

@robb robb added this to the 2.0 milestone Mar 6, 2014
@robb robb self-assigned this Mar 6, 2014
@dcaunt
Copy link
Member Author

dcaunt commented Mar 6, 2014

Sure, though I'm not sure how best to test it. Is there a better way than checking serialization matches that expected for a given subclass?

@robb
Copy link
Member

robb commented Mar 6, 2014

Hmm, not sure there is a nice way without adding mocks. I guess using a subclass with an implicit transformer is the easiest way.

@jspahrsummers
Copy link
Member

Sorry, just catching up—is this still a WIP?

@dcaunt
Copy link
Member Author

dcaunt commented Mar 13, 2014

I'm afraid so. Just need to sort out a test but the implementation is done.

On Thursday, 13 March 2014, Justin Spahr-Summers notifications@github.com
wrote:

Sorry, just catching up—is this still a WIP?


Reply to this email directly or view it on GitHubhttps://github.com//pull/258#issuecomment-37582161
.

@robb
Copy link
Member

robb commented Mar 13, 2014

I believe it's still missing a test to verify the use of subclasses

On Mar 13, 2014, at 21:24, Justin Spahr-Summers notifications@github.com wrote:

Sorry, just catching up—is this still a WIP?


Reply to this email directly or view it on GitHub.

@jspahrsummers
Copy link
Member

🆒 No worries, just checking.

@dcaunt
Copy link
Member Author

dcaunt commented Apr 11, 2014

Do you think we should add OCMock to verify this? I can't come up with a test that doesn't feel really hacky.

@robb
Copy link
Member

robb commented Apr 11, 2014

I'd rather not. What's wrong with adding a subclass like MTLTestJSONAdapter?

@dcaunt dcaunt unassigned robb Apr 16, 2014
@robb
Copy link
Member

robb commented Aug 31, 2014

@dcaunt are still interested in working on this?

@jspahrsummers
Copy link
Member

Superseded by #474.

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.

None yet

3 participants