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

Updating include syntax to support hasAndBelongsToMany relationships #368

Merged
merged 8 commits into from
Jun 21, 2014
Merged

Updating include syntax to support hasAndBelongsToMany relationships #368

merged 8 commits into from
Jun 21, 2014

Conversation

mshick
Copy link
Contributor

@mshick mshick commented Feb 3, 2014

I hope this is a minimally invasive change to support hasAndBelongsToMany relationships when using the include style syntax. I dangle an additional reference on the relation model, modelThrough and use that in the include logic; branching to a second include call or to returning a normal modelTo.all callback.

I also handled all the jshint errors I was getting in lib/include.js.

@1602
Copy link
Owner

1602 commented Feb 3, 2014

Wow. You did a good job on rewriting this code. I see no problems after brief look. As for tests, I agree with you, test/include.js is proper place for checking this logic. Thanks!

@mshick
Copy link
Contributor Author

mshick commented Feb 3, 2014

Okay, I got a simple test in there. Thanks for checking it out!

@Anachron
Copy link

Ping @1602

@faceleg
Copy link

faceleg commented Jun 21, 2014

@1602 please review?

1602 added a commit that referenced this pull request Jun 21, 2014
Updating include syntax to support hasAndBelongsToMany relationships
@1602 1602 merged commit ae290a6 into 1602:master Jun 21, 2014
@faceleg
Copy link

faceleg commented Jun 23, 2014

THANKS! 💃

pocesar pushed a commit to pocesar/promised-jugglingdb that referenced this pull request Jun 24, 2014
Updating include syntax to support hasAndBelongsToMany relationships
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.

4 participants