Skip to content

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Aug 31, 2016

Intended to simplify work in #1857

@mention-bot
Copy link

@bf4, thanks for your PR! By analyzing the annotation information on this pull request, we identified @beauby, @ehsanyousefi and @groyoh to be potential reviewers

@bf4
Copy link
Member Author

bf4 commented Aug 31, 2016

@NullVoxPopuli Where do you think a regression test for something like #1857 (comment) should go?

@NullVoxPopuli
Copy link
Contributor

@bf4, maybe in test/active_model_serializers/adapter/json_api/{type_test.rb, belongs_to_test.rb}?

It looks like both test files don't do anything with polymorphic belongs_to

# Association.new(:comments, { serializer: CommentSummarySerializer })
#
Association = Struct.new(:name, :serializer, :options, :links, :meta) do
class Association < Field
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@NullVoxPopuli
Copy link
Contributor

I'm a fan overall.
Also, looks like git can't figure out that files moving (#1898) doesn't mean there has to be conflicts

@bf4 bf4 force-pushed the make_association_a_field branch from 032b078 to 6956c47 Compare August 31, 2016 13:18
@bf4
Copy link
Member Author

bf4 commented Aug 31, 2016

@NullVoxPopuli rebased

#1897 (comment)

I"m not sure what your comment is referring to. (The file is test/adapter/json_api/ btw)

@NullVoxPopuli
Copy link
Contributor

I"m not sure what your comment is referring to. (The file is test/adapter/json_api/ btw)

you asked:

@NullVoxPopuli Where do you think a regression test for something like #1857 (comment) should go?

? or did you mean something else?

@bf4
Copy link
Member Author

bf4 commented Aug 31, 2016

Looks like the inflection testing in test/adapter/json_api/type_test.rb is really testing lib/active_model_serializers/adapter/json_api/resource_identifier.rb

@NullVoxPopuli
Copy link
Contributor

Looks like the inflection testing in test/adapter/json_api/type_test.rb is really testing lib/active_model_serializers/adapter/json_api/resource_identifier.rb

That should probably move to the test file for resource_identifier then.

@bf4
Copy link
Member Author

bf4 commented Aug 31, 2016

@NullVoxPopuli oh, github presentation of doc led me not to see your comment as a response to mine 🤦

@bf4
Copy link
Member Author

bf4 commented Aug 31, 2016

I'm not planning any more changes for this PR. Anything you suggest?

@NullVoxPopuli
Copy link
Contributor

Nope, this is a good chunk of good changes :-)

I like when things get less convoluted :-)

I'll merge when CI passes

@NullVoxPopuli NullVoxPopuli merged commit 20e394d into rails-api:master Aug 31, 2016
@bf4 bf4 deleted the make_association_a_field branch August 31, 2016 13:38
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