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

Require ActiveSupport's string inflections #1488

Merged

Conversation

nate00
Copy link
Contributor

@nate00 nate00 commented Jan 31, 2016

Active Model Serializers depends on Active Support's string inflections but doesn't require them. Thus, requiring this gem raises an exception. To reproduce:

require 'bundler/setup'
require 'active_model_serializers'

This code raises an exception:

/active_model_serializers/lib/active_model/serializer/adapter.rb:49:in `register': undefined method `underscore' for "Null":String (NoMethodError)
        from /active_model_serializers/lib/active_model/serializer/adapter/base.rb:7:in `inherited'
        from /active_model_serializers/lib/active_model/serializer/adapter/null.rb:4:in `<module:Adapter>'
        from /active_model_serializers/lib/active_model/serializer/adapter/null.rb:3:in `<class:Serializer>'
        from /active_model_serializers/lib/active_model/serializer/adapter/null.rb:2:in `<module:ActiveModel>'
        from /active_model_serializers/lib/active_model/serializer/adapter/null.rb:1:in `<top (required)>'
        from /active_model_serializers/lib/active_model/serializer/adapter.rb:85:in `require'
        from /active_model_serializers/lib/active_model/serializer/adapter.rb:85:in `<module:Adapter>'
        from /active_model_serializers/lib/active_model/serializer/adapter.rb:3:in `<class:Serializer>'
        from /active_model_serializers/lib/active_model/serializer/adapter.rb:2:in `<module:ActiveModel>'
        from /active_model_serializers/lib/active_model/serializer/adapter.rb:1:in `<top (required)>'
        from /active_model_serializers/lib/active_model/serializer.rb:24:in `require'
        from /active_model_serializers/lib/active_model/serializer.rb:24:in `<class:Serializer>'
        from /active_model_serializers/lib/active_model/serializer.rb:17:in `<module:ActiveModel>'
        from /active_model_serializers/lib/active_model/serializer.rb:16:in `<top (required)>'
        from /active_model_serializers/lib/active_model_serializers.rb:20:in `require'
        from /active_model_serializers/lib/active_model_serializers.rb:20:in `<module:ActiveModelSerializers>'
        from /active_model_serializers/lib/active_model_serializers.rb:4:in `<top (required)>'
        from test.rb:2:in `require'
        from test.rb:2:in `<main>'

@nate00
Copy link
Contributor Author

nate00 commented Jan 31, 2016

The tests are passing in master even though require 'active_model_serializers' raises an exception in a vanilla Ruby environment. That's because test_helper.rb requires Rails (which in turn requires Active Support) before it requires Active Model Serializers.

It would be nice to write tests to detect such missing requires. I'm not yet familiar enough with the code and the tests to know the best way write those tests. Maybe the existing tests should be divided into Rails and non-Rails tests, and we only require 'rails' for the Rails-specific tests?

@nate00 nate00 force-pushed the require-active-support-string-inflections branch from f54bd69 to dc4bdeb Compare January 31, 2016 07:52
@bf4
Copy link
Member

bf4 commented Jan 31, 2016

Maybe the existing tests should be divided into Rails and non-Rails tests, and we only require 'rails' for the Rails-specific tests?

Quite possible. Thanks for this!

@bf4
Copy link
Member

bf4 commented Jan 31, 2016

Mind adding this PR to the top of the fixes section in the changelog?

We depend on string/inflections to define String#underscore.
@nate00 nate00 force-pushed the require-active-support-string-inflections branch from dc4bdeb to 3c1fe0f Compare January 31, 2016 21:46
@nate00
Copy link
Contributor Author

nate00 commented Jan 31, 2016

@bf4 Thanks for the quick response! I just added it to the changelog. Did I do that right?

bf4 added a commit that referenced this pull request Jan 31, 2016
…flections

[FIX] Require ActiveSupport's string inflections
@bf4 bf4 merged commit 0edf488 into rails-api:master Jan 31, 2016
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.

2 participants