-
Notifications
You must be signed in to change notification settings - Fork 112
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 for defining default transformer #222
Conversation
lib/blueprinter/view.rb
Outdated
@@ -4,12 +4,12 @@ module Blueprinter | |||
class View | |||
attr_reader :excluded_field_names, :fields, :included_view_names, :name, :transformers, :definition_order | |||
|
|||
def initialize(name, fields: {}, included_view_names: [], excluded_view_names: [],transformers: []) | |||
def initialize(name, fields: {}, included_view_names: [], excluded_view_names: [], transformers: [Blueprinter.configuration.transform_default].compact) |
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.
The way this would work, the config-set transformer is really more of "base transform" than a default transform.
i.e. in this approach, the transform is not overwritten by the view-level transform.
I think it may be more flexible if the transform_default
was made something like default_transformers = []
and was over-writeable. So instead, perhaps implement the transformers
accessor as:
def transformers
transformers.empty? Blueprinter.configuration.default_transformers : transformers
end
The pro here is that a user can specify any amount of base transforms to be ran by default, but if a user wants to override this at the view level, they can.
The con here is if the user wants to add to the default_transforms instead of override, they'd need to specify all the default transforms again in addition to the added one(s).
I think this con may still be ok as the alternative would mean there isn't really a great way for users to override the default_transformers.
Thoughts @philipqnguyen and @supremebeing7 ?
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.
Ah you're right, I had actually tested that scenario locally but I'm realizing I didn't test it explicitly enough. So yes, I would agree on that being the preferred behavior. I will go ahead and implement that, unless/until @philipqnguyen has a different opinion.
Done, let me know what you all think. |
lib/blueprinter/view.rb
Outdated
@name = name | ||
@fields = fields | ||
@included_view_names = included_view_names | ||
@excluded_field_names = excluded_view_names | ||
@transformers = transformers | ||
@transformers = transformers.empty? ? Blueprinter.configuration.default_transformers.clone : transformers |
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.
Had to make a copy with clone
because otherwise any time any view's transformers was modified, it would modify the default_transformers
on the config object. (Don't ask me how long that took to track down... 🤯 )
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.
Thanks for making these changes :)
We're much closer, but there's some edge cases in this logic unfortunately not caught by the unit test that we'll also likely want to cover later in the base_spec.rb
This logic should not belong in the initializer because in a view, when we add a transform, it will not override the blueprinter config, but will still add onto it (same thing for inheritance I believe)
Instead, we should keep track of the view-level transforms separately from the config transform, and on access-time is when we should choose which to use.
For example:
view.rb
def initialize(name, fields: {}, included_view_names: [], excluded_view_names: [], transformers: [])
...
@view_transformers = transformers
...
end
...
# At access time is when we choose which transformers to use
def transformers
view_transformers.empty? ? Blueprinter.configuration.default_transformers : view_transformers
end
...
# Need to make sure when we inherit, we only inherit the view-level transformers (don't wan't to duplicate the config-level one each time we inherit)
def inherit(view)
...
view.view_transformers.each do |transformer|
add_transformer(transformer)
end
end
...
def add_transformer(custom_transformer)
view_transformers << custom_transformer
end
This may be able to be cleaned up but hopefully the above snippet shows some of the edge-cases beyond initialization.
- We should have some tests to ensure that inheritance (I think there may be some specs in base_spec.rb around inheritance) properly inherits the view-level transformers.
- We should have some tests in base_render_examples to ensure that we properly override transformers when we define a Blueprinter that adds a transform.
Again, thank you so much for your contributions and please don't hesitate to reach out if you need any help on getting this to the finish line 🙂
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 think I understand. I just pushed a commit that should handle (2) above. (1) looks like it's already being tested in base_spec.rb:452? Or perhaps I am not understanding what you're asking for there.
lib/blueprinter/view.rb
Outdated
@definition_order = [] | ||
@sort_by_definition = Blueprinter.configuration.sort_fields_by.eql?(:definition) | ||
end | ||
|
||
def transformers | ||
view_transformers.empty? ? Blueprinter.configuration.default_transformers.clone : view_transformers |
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.
Now that we've separated these out, I don't think we need the .clone
right?
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.
Indeed! Good catch, fixed in 062456a
One small comment from me, but otherwise looks good! I'll take a look again after the holiday weekend and will get another maintainer to take a look as well. |
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 is a great addition, thank you!
Fixes #221