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

Support has_one to be compatible with 0.8.x #725

Merged
merged 2 commits into from
Mar 5, 2015

Conversation

ggordon
Copy link
Contributor

@ggordon ggordon commented Nov 11, 2014

The 0.8.x branch used has_one for associations, it looks like this was changed to belongs_to in 0.10.x. This patch adds support for has_one in addition to belongs_to for backwards compatibility.

@kurko kurko closed this Nov 12, 2014
@kurko kurko reopened this Nov 12, 2014
@kurko
Copy link
Member

kurko commented Nov 12, 2014

Sorry, wrong button.

@joshsmith
Copy link
Contributor

@ggordon would you mind updating the description with your reasoning behind this?

@ggordon
Copy link
Contributor Author

ggordon commented Feb 17, 2015

@joshsmith done.

@joshsmith
Copy link
Contributor

Is the belongs_to association no longer tested? It looks like you removed the associations test for that.

@ggordon
Copy link
Contributor Author

ggordon commented Feb 17, 2015

I only added tests for has_one, I didn't delete anything.

@@ -44,7 +44,7 @@ def test_has_many
assert_equal(
{ posts: { type: :has_many, options: { embed: :ids } },
roles: { type: :has_many, options: { embed: :ids } },
bio: { type: :belongs_to, options: {} } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I misunderstanding, or is this not removing the the test for the belongs_to?

@ggordon
Copy link
Contributor Author

ggordon commented Feb 17, 2015

Sorry it's been a while since I looked at this... That change is in the 'test_has_many' method so it should be irrelevant. The test 'test_belongs_to' is testing the belongs_to association on the comment serializer, which hasn't changed.

@joshsmith
Copy link
Contributor

You don't think this belongs in a test_has_one? Just curious, since it feels kind of hidden for someone without all the context of this PR and reasoning behind it.

@ggordon
Copy link
Contributor Author

ggordon commented Feb 17, 2015

I could add a test to associations_test.rb, I guess I was only focused on jsonapi so I put it in adapter/json_api/has_one_test.rb

@joshsmith
Copy link
Contributor

Sure, whatever you think is best.

Appreciate the backwards compatibility on this, though! 💌

@ggordon
Copy link
Contributor Author

ggordon commented Feb 17, 2015

Didn't see any easy way to separate the has_many and has_one tests, so i renamed it to test_has_many_and_has_one.

@joshsmith
Copy link
Contributor

Awesome, that makes sense to me.

@kurko
Copy link
Member

kurko commented Mar 1, 2015

@ggordon could you update the README.md and CHANGELOG.md?

@kurko kurko added the V: 0.10.x label Mar 1, 2015
@ggordon
Copy link
Contributor Author

ggordon commented Mar 1, 2015

README.md and CHANGELOG.md updated. Also I was sometimes getting a failure in unrelated code, I think it was an id collision in the Post model ids and the order the tests were executed. I had to make a workaround to get the tests to consistently pass.

kurko added a commit that referenced this pull request Mar 5, 2015
Support has_one to be compatible with 0.8.x
@kurko kurko merged commit 3389218 into rails-api:master Mar 5, 2015
@ggordon ggordon deleted the has_one_support branch March 5, 2015 23:11
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 25, 2015
@bf4 bf4 mentioned this pull request Jun 25, 2015
aaronlerch pushed a commit to aaronlerch/active_model_serializers that referenced this pull request Jun 26, 2015
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.

3 participants