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

Allow for defining default transformer #222

Merged
merged 5 commits into from
Jul 6, 2020

Conversation

supremebeing7
Copy link
Contributor

Fixes #221

@@ -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)
Copy link
Contributor

@mcclayton mcclayton Jul 1, 2020

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 ?

Copy link
Contributor Author

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.

@supremebeing7
Copy link
Contributor Author

Done, let me know what you all think.

@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
Copy link
Contributor Author

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... 🤯 )

Copy link
Contributor

@mcclayton mcclayton Jul 2, 2020

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.

  1. 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.
  2. 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 🙂

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 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.

@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
Copy link
Contributor

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?

Copy link
Contributor Author

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

@mcclayton
Copy link
Contributor

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.

Copy link
Contributor

@adamwells adamwells left a 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!

@mcclayton mcclayton merged commit b418614 into procore-oss:master Jul 6, 2020
@mcclayton mcclayton mentioned this pull request Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow defining a transformer used by all views in a blueprint
3 participants