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

Fix related item import fallback keys #1228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix related item import fallback keys #1228

wants to merge 1 commit into from

Conversation

djbe
Copy link

@djbe djbe commented May 1, 2016

Implements #1170, without the extra code.

Implements #1170, without the extra code.
@Coeur
Copy link
Collaborator

Coeur commented Jun 21, 2019

Related: #1169

@Coeur Coeur self-assigned this Jun 21, 2019
@Coeur
Copy link
Collaborator

Coeur commented Jun 22, 2019

You wrote:

On that note, the MR_lookupKeyForAttribute: method might need some changing. At least the parameter type should be changed from NSAttributeDescription to NSPropertyDescription, to correctly match both use cases.

But you did not apply this change in your Pull Request.

Well, thank you for the Pull Request, and sorry for looking at it so late, but a unit test demonstrating the resolved issue is needed before I merge any fix.

@djbe
Copy link
Author

djbe commented Jun 22, 2019

Wow, talk about a blast from the past 😆

I haven't used this library in a few years, so I don't have any files ready to use as a unit test. I don't even remember in which of my projects I encountered this issue 😅

But I can guess from the original issue's description what the issue was (fallback import paths for relationships). If I find some time for this, I'll take a stab at some unit tests.

@Coeur
Copy link
Collaborator

Coeur commented Jun 22, 2019

Wow, talk about a blast from the past 😆

Ik weet.
The repository has been abandoned, but I recently found the time to review every pending pull request (for the 2.x version of MagicalRecord). It's too much work to create unit tests myself for every case, so I'll be conservative and I won't merge until then.

@Coeur Coeur removed their assignment Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants