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

Inherit view definition when using inheritance #105

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/blueprinter/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ def self.identifier(method, name: method, extractor: AutoExtractor)
view_collection[:identifier] << Field.new(method, name, extractor, self)
end

def self.inherited(subclass)
subclass.send(:view_collection).inherit(view_collection)
end

# Specify a field or method name to be included for serialization.
# Takes a required method and an option.
#
Expand Down
18 changes: 14 additions & 4 deletions lib/blueprinter/view.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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 added a spec for the subclassing scenario and modified the existing spec for the regular overriding scenario. Let me know what you think

end

view.included_view_names.each do |view_name|
include_view(view_name)
end

view.excluded_field_names.each do |field_name|
exclude_field(field_name)
end
end

def include_view(view_name)
included_view_names << view_name
end
Expand All @@ -19,10 +33,6 @@ def exclude_field(field_name)
end

def <<(field)
if fields.has_key?(field.name)
raise BlueprinterError,
"Field #{field.name} already defined on #{name}"
end
fields[field.name] = field
end
end
Expand Down
6 changes: 6 additions & 0 deletions lib/blueprinter/view_collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ def initialize
}
end

def inherit(view_collection)
view_collection.views.each do |view_name, view|
self[view_name].inherit(view)
end
end

def has_view?(view_name)
views.has_key? view_name
end
Expand Down
61 changes: 61 additions & 0 deletions spec/integrations/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,4 +141,65 @@
end
end
end

describe 'Using the ApplicationBlueprint pattern' do
let(:obj) { OpenStruct.new(id: 1, name: 'Meg', age: 32) }
let(:application_blueprint) do
Class.new(Blueprinter::Base) do
identifier :id
field :name
field(:overridable) { |o| o.name }

view :with_age do
field :age
end

view :anonymous_age do
include_view :with_age
exclude :name
end
end
end

let(:blueprint) do
Class.new(application_blueprint) do
field(:overridable) { |o| o.age }

view :only_age do
include_view :with_age
exclude :name
end
end
end

subject { blueprint.render_as_hash(obj) }

it('inherits identifier') { expect(subject[:id]).to eq(obj.id) }
it('inherits field') { expect(subject[:name]).to eq(obj.name) }
it('overrides field definition') { expect(subject[:overridable]).to eq(obj.age) }

describe 'Inheriting views' do
let(:view) { :with_age }
subject { blueprint.render_as_hash(obj, view: view) }

it('includes identifier') { expect(subject[:id]).to eq(obj.id) }
it('includes base fields') { expect(subject[:name]).to eq(obj.name) }
it('includes view fields') { expect(subject[:age]).to eq(obj.age) }

describe 'With complex views' do
let(:view) { :anonymous_age }

it('includes identifier') { expect(subject[:id]).to eq(obj.id) }
it('includes include_view fields') { expect(subject[:age]).to eq(obj.age) }
it('excludes excluded fields') { expect(subject).to_not have_key(:name) }
end

describe 'Referencing views from parent blueprint' do
let(:view) { :only_age }

it('includes include_view fields') { expect(subject[:age]).to eq(obj.age) }
it('excludes excluded fields') { expect(subject).not_to have_key(:name) }
end
end
end
end
17 changes: 11 additions & 6 deletions spec/units/view_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,14 @@
end

context 'Given a field that already exists' do
let(:aliased_field) { MockField.new(:fname, :first_name) }

before { view << field }
it { expect { view << field }.to raise_error(blueprinter_error) }
it 'should not set #fields' do
expect(view.fields).to eq({first_name: field})

it 'overrides previous definition' do
view << aliased_field

expect(view.fields).to eq(first_name: aliased_field)
end
end
end
Expand All @@ -56,8 +60,9 @@
end

class MockField
attr_reader :name
def initialize(name)
@name = name
attr_reader :name, :method
def initialize(method, name = nil)
@method = method
@name = name || method
end
end