search

ccocchi / rabl-rails

Rails 4.2+ templating system with JSON, XML and Plist support.
MIT License
209 stars 51 forks source link

Adding 'Enumerable' to collection criteria #83

Closed pkuykendall closed 8 years ago

pkuykendall commented 8 years ago

I made this modification to help resolve an issue I was having with my models. They respond to #each, but having them treated as a collection was breaking my views.

I added the Enumerable condition since most items that might be iterated on probably inherit from it at some point. Adding it before the non_collection_classes allows it to exit before checking the blacklist (which saves time, I know this was a concern in #55). Leaving in the blacklist lets us disallow any classes that might still cause problems.

Please let me know what you think.

ccocchi commented 8 years ago

Wasn't adding your model's classes to non_collection_classes enough to not have them bahave like a collection?

pkuykendall commented 8 years ago

Yes, but there was a big performance hit. As you guys discussed in #55, the string matching on class names takes a while. My ORM models all respond to each, so that would result in a lookup against the blacklist for every model if I used the non_collection_classes method.

Using the class check for Enumerable allows you to bypass the expensive string lookup. I left it in to still allow you to blacklist classes, if needed. I'm not sure if there are any other cases out there, but I can't think of any collection-type objects that don't inherit from Enumerable. Though, I might be missing something. 😄

Just a thought. What do you think?

ccocchi commented 8 years ago

Problem is that Enumerable is not good enough delimiter. As it may be a good practice to include it in your collection-like object, many real case objects aren't including it and are just defining an each method, so I don't see including this by default.

As for performance, I guess all your ORM models have a common class you can include to non_collection_classes, so at the end your classes are as fast as an Array would be. I wouldn't worry about this method as it is not where the gem spend most of its time.

If you really want to I guess you can monkey patch the RablRails::Helpers module.

pkuykendall commented 8 years ago

I can agree with that. Adding the Enumerable check works for my use cases, but I wouldn't want to interfere with any common practices.

Thanks for taking a look at this PR. I'll go ahead and close it out.