-
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
Allow transformers to be included across views #372
Changes from 9 commits
2d3c9c7
e3c8ecc
8815272
dec674f
1bcae06
8e20388
0acf5d1
1ca67b2
c723f9e
04564e6
fe71331
23b167e
6d39a88
012a8dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,9 @@ def fields_for(view_name) | |
end | ||
|
||
def transformers(view_name) | ||
views[view_name].transformers | ||
included_transformers = gather_transformers_from_included_views(view_name) | ||
all_transformers = included_transformers.concat(views[:default].view_transformers).uniq | ||
all_transformers.empty? ? Blueprinter.configuration.default_transformers : all_transformers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make a distinction between global default transformers and transformers of the default view. According to this section of the README, global default transformers should only be used if there are no other transformers. That's fine, and I've moved that logic here. But I don't believe transformers of the default view should be treated in the same way, which I've changed from how #363 handles it. Instead, transformers of the default view should be merged with the views' other transformers. This is the behavior I would expect, since fields on the default view are merged with the views' other fields. I've added tests for both of these situations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an edge case, but in what order should the transformers be applied? I would expect ones from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @njbbaer Currently, the default views transformer is not applied when calling with a specific view. Do we need to change this behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jhollinger I prefer from last to first in views because that provides easy tracing of transformers for the end implementer. If we taking that route can we also follow the same for default too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bhooshiek-narendiran I may not have defined my terms well enough. I was talking about "transformers from the default view", as opposed to "the default transformers configuration setting". I think @njbbaer's approach of ignoring "the default transformers configuration setting" if any transformers are defined in the Blueprint is reasonable. All discussion of "default" below refers to "transformers from the default view". Allow me to amend my comment. In the PR's current code, when transformers are present, they appear to be applied in this order:
That ordering strikes me as arbitrary, and I could see it leading to confusion in some edge cases. IMHO a much more predictable ordering would be "descending":
@bhooshiek-narendiran suggested the reverse of that order, which I also think would be fine:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @jhollinger, that's an important point. I'm inclined towards the "descending" order, which gives named views the opportunity to have the final say on their rendering, so to speak. @bhooshiek-narendiran can you elaborate on why you prefer the reverse?
Yes, we want to change this behavior so that it's easier to apply transformers more broadly. When a field is added to the default view it is applied to all of the named views, likewise a transformer added to the default view should also be applied in addition to the named views. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @njbbaer @jhollinger Apologize I also meant the same thing.
Execution order of transformer => But Adding it to first or last both seems to be a good addition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jhollinger @bhooshiek-narendiran @bhooshiek-narendiran Here's a more complete example, which uses included views:
The idea is that the "more specific" transformers get executed later, so they can get the last word on their rendering. |
||
end | ||
|
||
def [](view_name) | ||
|
@@ -89,5 +91,14 @@ def add_to_ordered_fields(ordered_fields, definition, fields, view_name_filter = | |
ordered_fields[definition.name] = fields[definition.name] | ||
end | ||
end | ||
|
||
def gather_transformers_from_included_views(view_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Should we make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it already? |
||
current_view = views[view_name] | ||
current_view.included_view_names.flat_map do |included_view_name| | ||
next [] if view_name == included_view_name | ||
|
||
gather_transformers_from_included_views(included_view_name) | ||
end.concat(current_view.view_transformers) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,10 @@ | |
|
||
$LOAD_PATH.unshift File.expand_path('../../lib', __FILE__) | ||
require 'blueprinter' | ||
require 'pry' | ||
require 'json' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope not! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added I added |
||
|
||
Dir[File.dirname(__FILE__) + '/support/**/*.rb'].each { |file| require file } | ||
|
||
module SpecHelpers | ||
def reset_blueprinter_config! | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# frozen_string_literal: true | ||
|
||
class MockField | ||
attr_reader :name, :method | ||
def initialize(method, name = nil) | ||
@method = method | ||
@name = name || method | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
# frozen_string_literal: true | ||
|
||
describe 'ViewCollection' do | ||
let(:view_collection) { Blueprinter::ViewCollection.new } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we use |
||
|
||
let!(:default_view) { view_collection[:default] } | ||
let!(:view) { view_collection[:view] } | ||
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a view collection tries to fetch a view that doesn't exist it creates it, which is what we're doing here. We need that view to exist for our tests, but don't always call |
||
|
||
let(:default_field) { MockField.new(:default_field) } | ||
let(:view_field) { MockField.new(:view_field) } | ||
let(:new_field) { MockField.new(:new_field) } | ||
|
||
before do | ||
default_view << default_field | ||
view << view_field | ||
end | ||
|
||
describe '#initialize' do | ||
it 'should create an identifier, view, and default view' do | ||
expect(view_collection.views.keys).to eq([:identifier, :default, :view]) | ||
end | ||
end | ||
|
||
describe '#[]' do | ||
it 'should return the view if it exists' do | ||
expect(view_collection.views[:default]).to eq(default_view) | ||
end | ||
|
||
it 'should create the view if it does not exist' do | ||
new_view = view_collection[:new_view] | ||
expect(view_collection.views[:new_view]).to eq(new_view) | ||
end | ||
end | ||
|
||
describe '#view?' do | ||
it 'should return true if the view exists' do | ||
expect(view_collection.view?(:default)).to eq(true) | ||
end | ||
|
||
it 'should return false if the view does not exist' do | ||
expect(view_collection.view?(:missing_view)).to eq(false) | ||
end | ||
end | ||
|
||
describe '#inherit' do | ||
let(:parent_view_collection) { Blueprinter::ViewCollection.new } | ||
|
||
before do | ||
parent_view_collection[:view] << new_field | ||
end | ||
|
||
it 'should inherit the fields from the parent view collection' do | ||
view_collection.inherit(parent_view_collection) | ||
expect(view.fields).to include(parent_view_collection[:view].fields) | ||
end | ||
end | ||
|
||
describe '#fields_for' do | ||
it 'should return the fields for the view' do | ||
expect(view_collection.fields_for(:view)).to eq([default_field, view_field]) | ||
end | ||
end | ||
|
||
describe '#transformers' do | ||
let(:transformer) { Blueprinter::Transformer.new } | ||
|
||
before do | ||
view.add_transformer(transformer) | ||
end | ||
|
||
it 'should return the transformers for the view' do | ||
expect(view_collection.transformers(:view)).to eq([transformer]) | ||
end | ||
|
||
it 'should not return any transformers for another view' do | ||
view_collection[:foo] | ||
expect(view_collection.transformers(:foo)).to eq([]) | ||
end | ||
|
||
context 'default view transformer' do | ||
let(:default_transformer) { Blueprinter::Transformer.new } | ||
|
||
before do | ||
default_view.add_transformer(default_transformer) | ||
end | ||
|
||
it 'should return the transformers for the default view' do | ||
expect(view_collection.transformers(:default)).to eq([default_transformer]) | ||
end | ||
|
||
it 'should return both the view transformer and default transformers for the view' do | ||
expect(view_collection.transformers(:view)).to eq([transformer, default_transformer]) | ||
end | ||
end | ||
|
||
context 'include view transformer' do | ||
let!(:includes_view) { view_collection[:includes_view] } | ||
let!(:nested_view) { view_collection[:nested_view] } | ||
|
||
before do | ||
includes_view.include_view(:view) | ||
end | ||
|
||
it 'should return the transformers for the included view' do | ||
expect(view_collection.transformers(:includes_view)).to include(transformer) | ||
end | ||
|
||
it 'should return the transformers for the nested included view' do | ||
nested_view.include_view(:includes_view) | ||
expect(view_collection.transformers(:nested_view)).to include(transformer) | ||
end | ||
|
||
it 'should only return unique transformers' do | ||
includes_view.add_transformer(transformer) | ||
transformers = view_collection.transformers(:nested_view) | ||
expect(transformers.uniq.length == transformers.length).to eq(true) | ||
end | ||
end | ||
|
||
context 'global default transformers' do | ||
let(:default_transformer) { Blueprinter::Transformer.new } | ||
|
||
before do | ||
Blueprinter.configure { |config| config.default_transformers = [default_transformer] } | ||
end | ||
|
||
context 'with no view transformers' do | ||
let!(:new_view) { view_collection[:new_view] } | ||
|
||
it 'should return the global default transformers' do | ||
expect(view_collection.transformers(:new_view)).to include(default_transformer) | ||
end | ||
end | ||
|
||
context 'with view transformers' do | ||
it 'should not return the global default transformers' do | ||
expect(view_collection.transformers(:view)).to_not include(default_transformer) | ||
end | ||
end | ||
end | ||
end | ||
end |
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.
This logic has been moved inside of
ViewCollection
since we need to aggregate the included transformers before deciding whether to apply the global default transformers. See #372 (comment) for details.