-
Notifications
You must be signed in to change notification settings - Fork 109
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
Inherit view definition when using inheritance #105
Inherit view definition when using inheritance #105
Conversation
This may not work when reopening the superclass:
Rendering Person in this case would not contain Not sure how this interacts with #94 , since that PR seems to redo the whole thing, but I've added integration tests that should cover the common use case. |
This is great work! I don't think we need to solve for the use case of re-opening the super class, at least not now. You can ignore #94, I'm still experimenting with that PR (I might close it for now actually so it doesn't cause confusion) |
@@ -10,6 +10,20 @@ def initialize(name, fields: {}, included_view_names: [], excluded_view_names: [ | |||
@excluded_field_names = excluded_view_names | |||
end | |||
|
|||
def inherit(view) | |||
view.fields.values.each do |field| | |||
self << field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @philipqnguyen for desired behavior, but due to the implementation of <<
in this class, we'd raise an error if a field name collides, however, in inheritance -- you'd expect those fields to be overridable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @mcclayton
I don't even know if we need to raise an error here https://github.com/procore/blueprinter/blob/master/lib/blueprinter/view.rb#L22-L25
Perhaps we can just remove that error all together? I never thought that error to be useful, and I don't think it is the responsibility of blueprinter to protect users from their own mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, would be nice to have some specs to ensure that the last defined field is the field implementation that gets set. Both in a single view with multiple duplicate field name implementations, as well for the case where multiple classes define a field, to ensure the lowest subclass implementation gets set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I'll add the specs and remove the guard, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a spec for the subclassing scenario and modified the existing spec for the regular overriding scenario. Let me know what you think
Enable field and view definitions on superclasses to be used by subclasses: class ApplicationBlueprint < Blueprinter::Base identifier :id field :created_at end class PersonBlueprint < ApplicationBlueprint fields :name, :age end Objects rendered with `PersonBlueprint` will contain fields `id`, `created_at`, `name`, and `age`.
When adding two definitions for the same field name to the same view, the last definition prevails. This also applies to inheriting views.
13fa8e1
to
7924cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you so much @hugopeixoto!
We'll put this into 0.7.0 |
🎉 |
Enable field and view definitions on superclasses
to be used by subclasses:
Objects rendered with
PersonBlueprint
will contain fieldsid
,created_at
,name
, andage
.(Addresses issue #80)