codenoble / cache-crispies

Speedy Rails JSON serialization with built-in caching
MIT License
156 stars 16 forks source link

Let subclassed serializers inherit attributes from the superclass #48

Open floriandejonckheere opened 1 year ago

floriandejonckheere commented 1 year ago

Allow subclassed serializers to inherit attributes from its parent, behaviour which is in line with the OOP paradigm. Attribute objects are not duplicated themselves, because they should be immutable (correct me if I'm wrong).

class PersonSerializer < CacheCrispies::Base
  serialize :name
end

class DeveloperSerializer < CacheCrispies::Base
  serialize :skills
end

developer = Developer.new(name: "John Doe", skills: [:ruby])

PersonSerializer.new(developer).as_json
# => { name: "John Doe" }

DeveloperSerializer.new(developer).as_json
# => { name: "John Doe", skills: ["ruby"] }
Flixt commented 1 year ago

Hey,

I'm not too sure about this change. It is a breaking change for existing projects if they have serializers inheriting from each other. It may not be common until now, because attributes would not have beed inherited, but probably people did it to inherit other functionality/private methods.

The same result as stated in the changed specs can be achieved with

  class CaloriesSerializer < CacheCrispies::Base
    serialize :calories
  end

  class NutritionSerializer < CacheCrispies::Base
    merge :itself, with: CaloriesSerializer
    serialize :fat,
              :carbohydrates,
              :sodium,
              :protein
  end

I'd argue it is even more powerful than the inheritance pattern because it already allows multiple serializers to add their attributes to the result.

In the end @adamcrown needs to decide what to do, but I'd tend to not increase the complexity of the gem to adding an additional way to achieve something that is already possible.

floriandejonckheere commented 1 year ago

Thanks, I didn't know about the merge :itself. That solves the problem indeed.

However, I'd still argue that inheriting attributes is a more OOP approach, and I was surprised to find out it didn't work like that. ActiveModel, for example, does allow inheriting attributes from the parent classes. You're right in that it's definitely a breaking change though.

@adamcrown What's your opinion on this?

adamcrown commented 1 year ago

I'm not opposed to making this change, but I do tend to think it's probably not worth the fact that it would be a breaking change, given how easy it is to get the same result.

I'll leave this open for further feedback. If there are enough requests to merge it, that's fine with me.