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

Generator now properly detects ApplicationSerializer #462

Merged
merged 2 commits into from
Feb 4, 2014

Conversation

fab
Copy link
Contributor

@fab fab commented Dec 3, 2013

Today I discovered that the gem supports the use of an ApplicationSerializer file that the other serializers can inherit from.

However, using the generator to create new serializers didn't have them inheriting from ApplicationSerializer like desired. It looks like the #defined? method isn't working like intended. (It returned nil unless I had already typed ::ApplicationSerializer into the console).

What I've proposed might not be the best way to do this but I can confirm that it works for both Ruby 1.9.3 and 2.0.0.

There isn't an issue filed for this because I thought it would be a better use of resources to just provide a fix.

Thoughts?

I'm also happy to edit the docs to point out the use of ApplicationSerializer. It's a great place to extract a method that appears in all your serializers. (Had this exact use case today)

@arthurnn
Copy link
Contributor

arthurnn commented Dec 4, 2013

I am not sure about this.

  1. rescue nil in general is a bad idea
  2. defined?(Foo) should be enough.

@fab
Copy link
Contributor Author

fab commented Dec 4, 2013

@arthurnn

  1. Just the first working thing that can to mind. Can you suggest an alternative?
  2. The generator doesn't work as intended on my local machine and changing the defined?(Foo) made the difference

@spastorino
Copy link
Contributor

Hey @fab thanks for reporting this ...
Can you provide a test case please?

@fab
Copy link
Contributor Author

fab commented Dec 31, 2013

@spastorino feel free to clone this project I've been working on: https://github.com/fab/benandjerrys_api
It's a Rails-API app that uses active_model_serializers.

I have created an ApplicationSerializer file and all my serializers inherit from ApplicationSerializer but when I do rails g serializer foo and examine the generated foo serializer it still inherits from ActiveModel::Serializer, not ApplicationSerializer as expected.

Using the fix I've submitted in this PR that bug doesn't occur.

@fab
Copy link
Contributor Author

fab commented Dec 31, 2013

ie. use gem 'active_model_serializers', :git => 'git://github.com/fab/active_model_serializers.git' in the Gemfile and then try rails g serializer bar and you'll see that it's fixed.

@spastorino
Copy link
Contributor

@fab 👍 I was asking if you could provide a test case like the ones you find here https://github.com/rails-api/active_model_serializers/blob/master/test/integration/generators/scaffold_controller_generator_test.rb but no worries I will do it :)

@fab
Copy link
Contributor Author

fab commented Jan 2, 2014

@spastorino whoops sorry, misunderstood you. Interestingly enough there were already test cases for this behaviour that must have been broken. I changed the method in which the presence of ApplicationSerializer is checked (not using rescue nil anymore) and have updated the respective tests accordingly. See the two new commits :)

Will go ahead and modify the docs too so people know that using ApplicationSerializer is an option.

@fab
Copy link
Contributor Author

fab commented Jan 3, 2014

@spastorino updated the docs. Let me know if you'd like anything done differently but otherwise I think this PR is finally good to go.

@spastorino
Copy link
Contributor

@fab I'd go back to the rescue nil approach. It sucks but due to Rails code reloading there's nothing else we can do.

@fab
Copy link
Contributor Author

fab commented Feb 1, 2014

@spastorino sorry I have been MIA for a while. Back on top of this now.

Good point about Rails code reloading. I've reverted the changes to use the rescue nil approach. The specs didn't need to be altered since they already work with this method. Fingers crossed that we can finally merge this.

spastorino added a commit that referenced this pull request Feb 4, 2014
Generator now properly detects ApplicationSerializer
@spastorino spastorino merged commit d985e64 into rails-api:master Feb 4, 2014
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

3 participants