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.

Sign up for GitHub

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

Jump to bottom

Fix #as_json glitch caused by JSON and Rails #1318

Closed
wants to merge 4 commits into from
Closed

Fix #as_json glitch caused by JSON and Rails #1318

wants to merge 4 commits into from

Conversation

mhuggins
Copy link

@mhuggins mhuggins commented Nov 3, 2015

This addresses issue #340 for the 0.8.x series. It looks like the existing fix for that issue was not included in that series of active_model_serializers.

bf4
bf4 reviewed Nov 5, 2015
View reviewed changes
lib/active_model/serializer.rb
@@ -338,7 +338,7 @@ def to_json(*args)
# object including the root.
def as_json(options={})
options ||= {}
if root = options.fetch(:root, @options.fetch(:root, root_name))
if root = options.to_hash.fetch(:root, @options.fetch(:root, root_name))
Copy link
Member

Choose a reason for hiding this comment

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

failing test?

Copy link
Author

Choose a reason for hiding this comment

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

I'll check it out tomorrow.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, these tests are already failing on 0.8.3 prior to this change.

Copy link
Author

Choose a reason for hiding this comment

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

For reference: https://travis-ci.org/rails-api/active_model_serializers/builds/43605004

Copy link
Author

Choose a reason for hiding this comment

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

It looks like the 0-8-stable branch has been failing since 2013, looks like this PR broke it: #477

Copy link
Author

Choose a reason for hiding this comment

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

@bf4 I fixed some of your gem's tests, so this is now in a better state than the current 0-8-stable branch. It's not passing on some older versions of ruby that you guys are testing on travis because:

mime-types requires Ruby version >= 1.9.2

as seen here.

Copy link
Member

Choose a reason for hiding this comment

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

@mhuggins s/my/our gem. Thanks for looking into this. Sorry for being slow here...

Copy link
Author

Choose a reason for hiding this comment

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

No problem, hopefully this is helpful and looks good. :)

@beauby
Copy link
Contributor

beauby commented Nov 11, 2015

Somebody familiar with the 0.8 codebase to review this? cc @joaomdmoura

@mhuggins
Copy link
Author

mhuggins commented Dec 8, 2015

Just thought I'd follow up to see if there's anything else I can offer here.

@beauby
Copy link
Contributor

beauby commented Dec 8, 2015

Considering ruby 1.8.7 has been EOLed a few years ago, and considering changes necessitating ruby >= 1.9.3 have been committed to the 0.8 branch, I believe we can safely remove support for ruby < 1.9.3, and merge this commit.
What I suggest is to add a commit that changes the .travis.yml to this PR, so that it also removes old rubies tests, and then keep on rocking.

@beauby
Copy link
Contributor

beauby commented Dec 8, 2015

Also, thanks a lot @mhuggins for taking the time to make this PR happen, and for following up!

@mhuggins
Copy link
Author

mhuggins commented Dec 8, 2015

Cool, happy to do that. I've got some work critical stuff this week, but I'll plan on updating this week. I appreciate you getting back to me! :)

@bf4 bf4 added the V: 0.8.x label Dec 14, 2015
@mhuggins
Copy link
Author

@beauby I've updated the .travis.yml file, and tests are now passing. In addition to removing 1.8.7 and 1.9.2, I had to remove ree for the same reason since it's built upon the 1.8 series.

Let me know if there's anything else that needs to be addressed. Thanks! 😄

bf4
bf4 reviewed Dec 14, 2015
View reviewed changes
test/serialization_test.rb
@@ -242,7 +242,7 @@ def test_render_json_with_status

def test_render_json_with_callback
get :render_json_hello_world_with_callback
assert_equal 'alert({"hello":"world"})', @response.body
assert_equal '/**/alert({"hello":"world"})', @response.body
Copy link
Member

Choose a reason for hiding this comment

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

oh weird, shouldn't it have been this the whole time?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, it is currently failing on the 0.8 branch. This fixes it.

@bf4
Copy link
Member

bf4 commented Dec 14, 2015

Seems like you'll need to add an appveyor.yml to get Travis to go green

@mhuggins
Copy link
Author

I've never used appveyor. The 0.8 branch was failing historically, so nothing new broke here. Any insight into what I can do to get that working?

@bf4 bf4 mentioned this pull request Dec 15, 2015
Merged
@bf4
Copy link
Member

bf4 commented Dec 15, 2015

@mhuggins I think the big problems with CI on the 0-8-branch were related to incompatibilities with Rails < 4, and the lack of a Rails version constraint in CI.

I've addressed those in #1376 basically copying what we have in master. Hopefully, that should let you focus on the bug your fixing.

I included all your CI-specific commits in #1376

@mhuggins
Copy link
Author

Cool, I think it's safe to close this PR in favor of that one then. Thanks!!

@mhuggins mhuggins closed this Dec 15, 2015
@bf4
Copy link
Member

bf4 commented Dec 15, 2015

Oh, but I left out 2dbc4dd

Do you think it's no longer needed?

@bf4
Copy link
Member

bf4 commented Dec 15, 2015

Do you think it's worth working on the ruby < 1.9.3, rails < 4.0 test support?

@bf4
Copy link
Member

bf4 commented Dec 15, 2015

In the referenced bug you wrote

I'm still seeing this error on Rails 3.2.22 with ActiveModel Serializers 0.8.3. Am I missing something?

    serializer = Service::Notifications::NewsroomSerializer.new(newsroom)
    s = serializer.to_json
    # => "{\"name\":\"Huggie\",\"subdomain\":\"huggie\"}" 

   JSON.dump(s)
    # => NoMethodError: undefined method `fetch' for #<JSON::Ext::Generator::State:0x007faf44dc3048>

   MultiJson.dump(s)
    # => "{\"name\":\"Huggie\",\"subdomain\":\"huggie\"}" 
    ```

Which I haven't confirmed if it's still a problem

@mhuggins
Copy link
Author

Ahh, okay, I misunderstood. I'll reopen a new PR with just that change then, if that works for you. :)

@bf4
Copy link
Member

bf4 commented Dec 15, 2015

No need, you can just force push to this branch, bug/json-dump, on mhuggins that way we get to reuse the discussion

@mhuggins
Copy link
Author

👍 Will do shortly

@mhuggins
Copy link
Author

mhuggins commented Jan 4, 2016

Sorry for the delay, I'm going to make sure to address this during the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
V: 0.8.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mhuggins @beauby @bf4