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 embed option for associations #691

Merged

Conversation

jacob-s-son
Copy link
Contributor

No description provided.

Fixed reference to association options
@guilleiguaran
Copy link
Member

👍

@kurko
Copy link
Member

kurko commented Oct 29, 2014

This is good but there were no broken tests, which is dangerous. Could you add a test for this?

@joaomdmoura
Copy link
Member

Agree with @kurko

@jacob-s-son
Copy link
Contributor Author

@kurko , could you clarify please?

@kurko
Copy link
Member

kurko commented Oct 29, 2014

@jacob-s-son you changed a piece of code and nothing broke, no test got red. As a good practice, we always write a failing test which will get green with the fix. In other words, you're missing tests.

@guilleiguaran
Copy link
Member

I've tested this locally and without this a test under test/adapter/json_api/has_many_embed_ids_test.rb is failing, looks like it wasn't failing just because the filename (the file was missing the _test suffix)

kurko added a commit that referenced this pull request Oct 29, 2014
@kurko kurko merged commit baf3db1 into rails-api:master Oct 29, 2014
@kurko
Copy link
Member

kurko commented Oct 29, 2014

Thanks, @guilleiguaran.

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

4 participants