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

Let subclassed serializers inherit attributes from the superclass #48

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

floriandejonckheere
Copy link
Contributor

@floriandejonckheere floriandejonckheere commented Aug 11, 2022

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
Copy link
Contributor

Flixt commented Aug 17, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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.

@adamcrown adamcrown added the looking for feedback Looking for community feedback on this issue label Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
looking for feedback Looking for community feedback on this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants