ruby-grape / grape-active_model_serializers

User active_model_serializers with Grape
MIT License
139 stars 68 forks source link

Use ActiveModelSerializers::Adapter #53

Closed knao124 closed 8 years ago

knao124 commented 8 years ago

Now, nothing happened even if we set ActiveModelSerializers.config.adapter value.

I think this line (https://github.com/ruby-grape/grape-active_model_serializers/blob/4190cbf18fabca646a08fa1af6ade202454e4e82/lib/grape-active_model_serializers/formatter.rb#L9) should be changed to use ActiveModelSerializers.config.adapter when it is set.

Seeing these files,

we should use ActiveModelSerializers::Adapter.create(serializer) instead of serializer.to_json to apply ActiveModelSerializers.config.adapter.

knao124 commented 8 years ago

I think this issue is related with this PR :) https://github.com/ruby-grape/grape-active_model_serializers/issues/52

dblock commented 8 years ago

For starters, the build is failing. It also will need an entry in CHANGELOG please.

fogisland commented 8 years ago

AMS had a break change on 0.10.1, thus almost all tests are in failure state now.

I fixed one error due to AMS api change without modify the test cases: https://github.com/fogisland/grape-active_model_serializers/commit/7810f48015cf5c1ddbf1c902953750da606860e6

but the remaining failures seem need to be fixed by modifying the test cases.

dblock commented 8 years ago

So this effectively is supposed to let one use AMS 0.10.1, right? If someone ^^^ makes a green pull request, updates CHANGELOG and documents what's going on here I'll merge.

drn commented 8 years ago

@dblock - I took the liberty of including this fix and a number of other issues caused by AMS changes: https://github.com/ruby-grape/grape-active_model_serializers/pull/54

drn commented 8 years ago

This change has been merged https://github.com/ruby-grape/grape-active_model_serializers/pull/54. Thanks @knao124