Allow for defining default transformer#222
Conversation
| 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.
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.
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. |
| @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.
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.
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
endThis 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.
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.
| end | ||
|
|
||
| def transformers | ||
| view_transformers.empty? ? Blueprinter.configuration.default_transformers.clone : view_transformers |
There was a problem hiding this comment.
Now that we've separated these out, I don't think we need the .clone right?
|
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. |
adamwells
left a comment
There was a problem hiding this comment.
This is a great addition, thank you!
Fixes #221