Skip to content

Commit

Permalink
Merge pull request #304 from rickcarlino/master
Browse files Browse the repository at this point in the history 
Opps.
  • Loading branch information
@RickCarlino
RickCarlino committed Nov 9, 2014
2 parents 2afde79 + b4a199d commit 73989f3
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion app/serializers/garden_serializer.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class GardenSerializer < ApplicationSerializer
# conform to JSONApi out of the box.
# There might be a solution here:
# https://github.com/rails-api/active_model_serializers/issues/646
embeds_many :garden_crops # ,
# embeds_many :garden_crops # ,
# embed: :ids,
# key: :garden_crops,
# embed_namespace: :links
Expand Down

6 comments on commit 73989f3

@simonv3
Copy link
Member

@simonv3 simonv3 commented on 73989f3 Nov 9, 2014

Choose a reason for hiding this comment

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

Hmmmmmm. This was causing the server to bug out?

@RickCarlino
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's why:

In development mode, Rails lazy loads classes. It won't run that file until it needs to. This would lead us to believe that everything is OK. In production, it loads everything into memory ahead of time. There is (afaik) no such thing as embeds_many for AMS so this class threw a NoMethodError as soon as it started up in production. Pretty sure you could just do a has_many and it will be good enough. I will poke around later.

@simonv3
Copy link
Member

Choose a reason for hiding this comment

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

Could we set it up so that testing could catch that?

I actually had a lot of issues with the whole embeds_many vs has_many shenanigans. You'll see traces of that here and there (no garden index page, all the commented-out code). It probably does need a second pair of (way more experienced) eyes.

@RickCarlino
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the solution is to just write unit tests for your serializers. The reason Rails has that feature set is so that you can write code and not reset the server everytime.

The weird thing is that our coverage percentage didn't seem to drop?? I am going to take a look at coveralss and see.

@RickCarlino
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look at this tomorrow morning (Time.now + 8.hours). If you could shoot me a message on G+ of line numbers that were suspect, I would really appreciate it.

@simonv3
Copy link
Member

Choose a reason for hiding this comment

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

The serializers get used whenever the API gets used, so that should explain why the coverage wouldn't have dropped? I'll send you a message with where I remember the problems being.

Please sign in to comment.