Pull Request: Fixed KVC relationship mapping bug #247

Merged
merged 1 commit into from Oct 16, 2012

Conversation

Projects
None yet
4 participants
Contributor

JRG-Developer commented Sep 7, 2012

Please pull this into master.

I spent hours trying to debug my own code before I realized that this bug existed in Magical Import.

There was only one line changed (see code diff), but it's pretty significant.

Thanks.

Member

blackgold9 commented Sep 7, 2012

Can you explain what this change does. Not familiar with this part of the code base. Also, adding a unit test that verifies this behavior would be great. Thanks!

Member

blackgold9 commented Sep 8, 2012

Do you have a code sample that triggered the bug and explain how this change fixes it? Don't want to roadblock you on a unit test, but I need to understand it before it get's committed.

Contributor

JRG-Developer commented Sep 9, 2012

Here's my stack overflow post on it:
http://stackoverflow.com/questions/12312888/magical-records-how-to-map-relationships-using-key-path

I will create a unit test for this too.

Member

blackgold9 commented Oct 15, 2012

Any progress on the unit test? I'd like to merge this.

cmezak commented Oct 15, 2012

crap. I just made a pull request for this. I'll see if I can come up with a unit test for my request.

cmezak commented Oct 15, 2012

…but in the meantime, @blackgold9, if you have a look at the KVC documentation, I think you'll be convinced that it's safe to merge this request. @JRG-Developer merely replaces valueForKey: with valueForKeyPath:. The latter simply splits the key by occurrences of '.' and calls valueForKey: recursively for each component.

Since KVC doesn't allow '.' as part of a key, in any case where valueForKey: returns a value, valueForKeyPath: should return the same value. @JRG-Developer's request simply makes this code more flexible – as it was intended to be.

Owner

casademora commented Oct 15, 2012

Was this merged? I just saw this now, and it seems closed.
It's ok by me to merge it. I'm surprised this was only found now :/

On Oct 15, 2012, at 5:23 PM, Charlie Mezak notifications@github.com wrote:

…but in the meantime, @blackgold9, if you have a look at the KVC documentation, I think you'll be convinced that it's safe to merge this request. @JRG-Developer merely replaces valueForKey: with valueForKeyPath:. The latter simply splits the key by occurrences of '.' and calls valueForKey: recursively for each component.

Since KVC doesn't allow '.' as part of a key, in any case where valueForKey: returns a value, valueForKeyPath: should return the same value. @JRG-Developer's request simply makes this code more flexible – as it was intended to be.


Reply to this email directly or view it on GitHub.

cmezak commented Oct 15, 2012

I had a separate pull request with the same change that I just closed. This one is still open and ready to be merged in when somebody pulls the trigger,

blackgold9 merged commit 272af9c into magicalpanda:master Oct 16, 2012

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