Grape formatter feature requested in #1258 - Rebased and Repushed (#1273) #1336

johnhamelink · 2015-11-19T13:45:39Z

I've rebased, squashed and repushed #1273 so we can get this merged!

adds handling for when the returned resource is not serializable via ams
fix for when resource is an Array
Moves grape include to grape namespace. Changes Enumerable to Array because a plain hash is enumerable.
Add integration test
Refine scope of Grape version dependency
Assert that the response is equal to a manually defined JSON string
Add single module to include in Grape projects
Create a Serializable Resource to test rails-api from Grape
Update docs
Fix discrepency between ActiveRecord 4.0 - 4.1 and 4.2
Remove different behaviour for Arrays
Clean up copy in documentation

Edit: Just want to explicitly point out that credit for code goes to Julian Paas (https://github.com/jpaas)

Todos:

Add instructions to docs
Use self.call instead of a singleton
Remove .gitignore
Add Unit tests
Add single module for including the formatter & helper in their Grape project
Assert JSON response & not the ability for the JSON parser to parse it (see here)
Fix tests for Rails < 4.1
Squash & Rebase

bf4 · 2015-11-19T18:48:05Z

lib/grape/formatters/active_model_serializers.rb

+  module Formatters
+    module ActiveModelSerializers
+      class << self
+        def call(resource, env)


Look great.

would you mind changing this to def self.call(resource, env)? I don't think there's any advantage to opening the singleton class here

could you add the documentation from lib/grape/active_model_serializers.rb to the docs or README as you see fit. (I think there's an integrations page in progress somewhere)

Could you add a test so we can better support this code?

@bf4 I'll get that done for you, hopefully within the next 48 hours.

Fantastic. I apologize for not bring it up earlier. Somehow didn't see it

beauby · 2015-11-19T23:47:53Z

.gitignore

@@ -21,3 +21,4 @@ tmp
 *.swp
 .ruby-version
 .ruby-gemset
+.idea


Non project-specific ignores do not belong in the .gitignore. You should pur them in your local ignore file.

True enough, but there's already non-project stuff in there

B mobile phone

On Nov 19, 2015, at 5:47 PM, Lucas Hosseini notifications@github.com wrote:

In .gitignore:

@@ -21,3 +21,4 @@ tmp
*.swp
.ruby-version
.ruby-gemset
+.idea
Non project-specific ignores do not belong in the .gitignore. You should pur them in your local ignore file.

—
Reply to this email directly or view it on GitHub.

True enough, but not reason to add more imho.

johnhamelink · 2015-11-20T01:16:25Z

@bf4 wrt to building a test, since it's not my code and I'm not familiar with how AMS works internally, I'm struggling to come up with a good strategy for testing effectively - could you give me some pointers so I don't waste time on a crappy test?

bf4 · 2015-11-20T15:07:59Z

Maybe take a look at how grape or a grape formatter tests it? You'd likely have to add grape as a dev dependency

B mobile phone

On Nov 19, 2015, at 7:16 PM, John Hamelink notifications@github.com wrote:

@bf4 wrt to building a test, since it's not my code and I'm not familiar with how AMS works internally, I'm struggling to come up with a good strategy for testing effectively - could you give me some pointers so I don't waste time on a crappy test?

—
Reply to this email directly or view it on GitHub.

johnhamelink · 2015-11-23T13:00:33Z

@bf4 I've implemented some tests which show that it works with Grape - I added one to show that adding metadata works, but the test fails on all Rails versions except for 4.1 - Could you provide insight as to why that might be? Is this a bug in Grape or in this PR?

bf4 · 2015-11-23T15:49:03Z

active_model_serializers.gemspec

@@ -47,4 +47,5 @@ Gem::Specification.new do |spec|

  spec.add_development_dependency 'bundler', '~> 1.6'
  spec.add_development_dependency 'timecop', '~> 0.7'
+  spec.add_development_dependency 'grape', '~> 0.13.0'


Just curious, do you mean to be 0.13.x where x >= 0 or 0.x where x >= 13?. If the latter, it should be ~> 0.13

The latter. Thanks for pointing that out 😄

Edit: Done

bf4 · 2015-11-23T20:22:30Z

Thanks for all your work on this. I'm hoping you can figure out the build failures so I don't have to... a quick look makes it seem it's not using the json api adapter.

johnhamelink · 2015-11-24T00:14:27Z

@bf4 Figured it out - there's a difference between how ActiveRecord 4.2 and 4.0 - 4.1 handle resource.respond_to?(:model_name). I added a check for both resource.respond_to?(:model_name) and resource.class.respond_to?(:model_name) and that cleaned things up. Just waiting for all the specs to pass now 🍏

bf4 · 2015-11-24T17:26:47Z

CHANGELOG.md

@@ -14,6 +14,7 @@ Breaking changes:

 Features:

+- [#1336](https://github.com/rails-api/active_model_serializers/pull/1336) Added support for Grape 0.13.x


should be >= 0.13, < 1.0

per + spec.add_development_dependency 'grape', '~> 0.13'

bf4 · 2015-11-24T17:33:49Z

lib/grape/formatters/active_model_serializers.rb

+  module Formatters
+    module ActiveModelSerializers
+      def self.call(resource, env)
+        return resource.to_json unless resource.is_a?(Array) || resource.respond_to?(:model_name) || resource.class.respond_to?(:model_name)


just thinking out loud: might be easier to read as

if resource.is_a... resource.to_json else ..new(..).to_json end

Naive question: ActiveModelSerializers should handle Arrays and non-models just fine. See the logic in ActionController::Serialization. Any reason you're checking for an Array or model_name?

@bf4 that was put there by the previous developer, I have no idea why it was there, but assumed it was OK due to it being in the previous PR

Ok. I just never noticed it till now. Progressive review I guess?

😄 better caught now than in upstream master. I'll remove it

johnhamelink · 2015-11-24T17:34:06Z

Made the documentation changes mentioned in inline comments, rebased & pushed 😄

bf4 · 2015-11-26T02:04:43Z

ref: #1258

bf4 · 2015-11-26T04:07:45Z

Ok, rebase off of master and they should pass #1349

johnhamelink · 2015-11-30T10:29:28Z

@bf4 All good to merge?

bf4 · 2015-12-09T20:12:55Z

@johnathanludwig I'm sorry this hasn't been merged. I'd prefer to get a second LGTM on it, so @rails-api/ams any objects to merging this? 24 hours till merge.

Also, you'll have to rebase it off master and resolve conflicts. Let me know if you'd like help.

johnathanludwig · 2015-12-09T20:43:26Z

@bf4 I think you meant to tag @johnhamelink

johnhamelink · 2015-12-10T01:11:03Z

@bf4 I've rebased and resolved the conflicts (sensibly I hope). Let me know if you've any further comments!

bf4 · 2015-12-10T04:57:07Z

active_model_serializers.gemspec

@@ -53,4 +53,5 @@ Gem::Specification.new do |spec|
  spec.add_development_dependency 'bundler', '~> 1.6'
  spec.add_development_dependency 'timecop', '~> 0.7'
  spec.add_development_dependency 'minitest-reporters'
+  spec.add_development_dependency 'grape', '~> 0.13'


should be [' >= 0.13', '< 1.0'

- adds handling for when the returned resource is not serializable via ams - fix for when resource is an Array - Moves grape include to grape namespace. Changes Enumerable to Array because a plain hash is enumerable. - Add integration test - Refine scope of Grape version dependency - Assert that the response is equal to a manually defined JSON string - Add single module to include in Grape projects - Create a Serializable Resource to test rails-api from Grape - Update docs - Fix discrepency between ActiveRecord 4.0 - 4.1 and 4.2 - Updated Changelog - Remove parens from `render`, use `serializable` in all tests.

bf4 · 2015-12-10T13:53:40Z

test/grape_test.rb

+  def test_formatter_handles_arrays
+    get '/grape/render_array_with_json_api'
+
+    expected = {


serializable? or I can merge it

How could I create an array to pass into the serializer?

If you modified the routing as

+ def setup + @post = ARModels::Post.create(title: 'Dummy Title', body: 'Lorem Ipsum') + ARModels::Post.create(title: 'post 2', body: 'body 2') + @posts = ARModels::Post.all + end # .. resources :grape do get '/render' do - post = ARModels::Post.new(title: 'Dummy Title', body: 'Lorem Ipsum') + render @post end get '/render_with_json_api' do - post = ARModels::Post.new(title: 'Dummy Title', body: 'Lorem Ipsum') - render post, meta: { page: 1, total_pages: 2 }, adapter: :json_api + render @post, meta: { page: 1, total_pages: 2 }, adapter: :json_api end get '/render_array_with_json_api' do - render ARModels::Post.all, adapter: :json_api + render @posts, adapter: :json_api end end

Then your tests become testing

serializable_resource = serializable(@post) serializable_resource = serializable(@post, serializer: ARModels::PostSerializer, adapter: :json_api, meta: { page: 1, total_pages: 2 }) serializable_resource = serializable(@posts, adapter: :json_api)

or expected = serializable_resource.to_json

Sorry I never got back to you on this - work got in the way 😄

I tried to implement what you said, but unfortunately it doesn't work due to the fact that @post can't be associated from within GrapeTest class. I say LGTM.

Grape formatter feature requested in #1258 - Rebased and Repushed (#1273)

bf4 · 2015-12-18T02:31:49Z

@johnhamelink merged, yay!

johnhamelink · 2015-12-18T07:47:01Z

🎉 hurray!

Thanks @bf4 for your great advice & guidance :)

bf4 reviewed Nov 19, 2015
View reviewed changes

bf4 mentioned this pull request Nov 19, 2015

Grape formatter feature requested in #1268 #1273

Closed

beauby reviewed Nov 19, 2015
View reviewed changes

johnhamelink force-pushed the master branch 4 times, most recently from b437365 to 7d83e1a Compare November 23, 2015 13:16

bf4 reviewed Nov 23, 2015
View reviewed changes

johnhamelink force-pushed the master branch from 624fe5c to abe0982 Compare November 24, 2015 00:12

johnhamelink force-pushed the master branch from abe0982 to d1d55f0 Compare November 24, 2015 00:37

johnhamelink mentioned this pull request Nov 24, 2015

Fix bundler caching in travis & Appveyor #1341

Merged

bf4 reviewed Nov 24, 2015
View reviewed changes

johnhamelink force-pushed the master branch from d3bfb8e to 323d9f7 Compare November 24, 2015 17:33

bf4 reviewed Nov 24, 2015
View reviewed changes

bf4 mentioned this pull request Nov 26, 2015

Grape Support #1258

Closed

bf4 mentioned this pull request Nov 26, 2015

Add more warnings ignore directories #1349

Merged

johnhamelink force-pushed the master branch 3 times, most recently from b1a9161 to 5c5d8ca Compare November 26, 2015 16:11

johnhamelink changed the title ~~Grape formatter feature requested in #1268 - Rebased and Repushed (#1273)~~ Grape formatter feature requested in #1258 - Rebased and Repushed (#1273) Nov 26, 2015

bf4 added C: Feature V: 0.10.x S: LGTM labels Nov 30, 2015

johnhamelink force-pushed the master branch from 5c5d8ca to 2e5214b Compare December 10, 2015 01:09

bf4 reviewed Dec 10, 2015
View reviewed changes

johnhamelink force-pushed the master branch from e53118c to d85a17b Compare December 10, 2015 10:07

bf4 reviewed Dec 10, 2015
View reviewed changes

bf4 added a commit that referenced this pull request Dec 18, 2015

Merge pull request #1336 from johnhamelink/master

dff607d

Grape formatter feature requested in #1258 - Rebased and Repushed (#1273)

bf4 merged commit dff607d into rails-api:master Dec 18, 2015

bf4 removed the S: LGTM label Dec 18, 2015

bf4 mentioned this pull request Dec 31, 2015

Consider evaluating association in serializer context #1378

Merged

		@@ -14,6 +14,7 @@ Breaking changes:

		Features:

		- [#1336](https://github.com/rails-api/active_model_serializers/pull/1336) Added support for Grape 0.13.x

Grape formatter feature requested in #1258 - Rebased and Repushed (#1273) #1336

Grape formatter feature requested in #1258 - Rebased and Repushed (#1273) #1336

Conversation

johnhamelink commented Nov 19, 2015

Todos:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhamelink commented Nov 20, 2015

bf4 commented Nov 20, 2015

johnhamelink commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Nov 23, 2015

johnhamelink commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhamelink commented Nov 24, 2015

bf4 commented Nov 26, 2015

bf4 commented Nov 26, 2015

johnhamelink commented Nov 30, 2015

bf4 commented Dec 9, 2015

johnathanludwig commented Dec 9, 2015

johnhamelink commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Dec 18, 2015

johnhamelink commented Dec 18, 2015