Allow transformers to be included across views #372

njbbaer · 2023-12-26T19:44:26Z

Allows transformers to be included by other views.

Branched off of #363 by @bhooshiek-narendiran and resolves #225.

Refactored logic for gathering included transformers.
Added a View Collection tests suite.
Fixed handling of global default transformers and transformers of the default view.

Checklist:

I have updated the necessary documentation
I have signed off all my commits as required by DCO
My build is green

Signed-off-by: bhooshiek <[email protected]>

Signed-off-by: Nate Baer <[email protected]>

spec/units/view_collection_spec.rb

…of replace them. - Enhance view collection test suite and address comments. Signed-off-by: Nate Baer <[email protected]>

lib/blueprinter/view_collection.rb

…f there are no view transformers. Signed-off-by: Nate Baer <[email protected]>

njbbaer · 2023-12-27T07:25:37Z

lib/blueprinter/view_collection.rb

+      all_transformers = included_transformers.concat(views[:default].view_transformers).uniq
+      all_transformers.empty? ? Blueprinter.configuration.default_transformers : all_transformers


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.

This is an edge case, but in what order should the transformers be applied? I would expect ones from the default view first, followed by included views, and finally any from the final view. Looks like this might apply the default view ones last. Thoughts?

@njbbaer Currently, the default views transformer is not applied when calling with a specific view. Do we need to change this behavior?

@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?

@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:

Transformers from intermediate views (top-down)

Transformers from the named view

Transformers from :default view

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":

Transformers from :default view

Transformers from intermediate views (top-down)

Transformers from the named view

@bhooshiek-narendiran suggested the reverse of that order, which I also think would be fine:

Transformers from the named view

Transformers from intermediate views (bottom-up)

Transformers from :default view

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?

Currently, the default views transformer is not applied when calling with a specific view. Do we need to change this behavior?

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.

@njbbaer @jhollinger Apologize I also meant the same thing.

class Sample transformer_view_default view_1 do transformer_1 end view_2 do transformer_2 end view_3 do transformer_3 end end

Execution order of transformer =>transformer_3,transformer_2,transformer_1
Do we need to merge transformer_view_default along with views transformer or only during views transformer not present.

But Adding it to first or last both seems to be a good addition.

@jhollinger @bhooshiek-narendiran
I've made the PR changes described, so the execution order is now "top-down".

@bhooshiek-narendiran
Not quite. In your example (without include statements) each view would execute these transformers in this order:
view_1 => transformer_view_default, transformer_1
view_2 => transformer_view_default, transformer_2
view_3 => transformer_view_default, transformer_3

Here's a more complete example, which uses included views:

class Sample transform TransformViewDefault view :view_1 do transform Transformer1 end view :view_2 do transform Transformer2 include :view_1 end view :view_3 do transform Transformer3 include :view_2 end end

view_1 => TransformViewDefault, Transformer1
view_2 => TransformViewDefault, Transformer1, Transformer2
view_3 => TransformViewDefault, Transformer1, Transformer2, Transformer3

The idea is that the "more specific" transformers get executed later, so they can get the last word on their rendering.

Signed-off-by: Nate Baer <[email protected]>

njbbaer · 2023-12-27T07:31:58Z

lib/blueprinter/view.rb

-    def transformers
-      view_transformers.empty? ? Blueprinter.configuration.default_transformers : view_transformers
-    end
-


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.

- Minor textual changes to view collection spec. Signed-off-by: Nate Baer <[email protected]>

lessthanjacob · 2023-12-29T19:56:34Z

lib/blueprinter/view_collection.rb

@@ -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)


nit: Should we make this private?

Isn't it already?

lessthanjacob · 2023-12-29T19:59:16Z

spec/spec_helper.rb

+require 'pry'
+require 'json'


Are these require's necessary?

I hope not! :)

I added require 'json' here because without it many spec files fail when run individually (i.e. bundle exec rspec spec/units/view_spec.rb) because JSON is uninitialized. The test suite works currently when run in its entirety because of the order of the specs. When we use active record in the integration tests, which run first, it imports JSON and allows the subsequent tests to pass. Is it a bad idea to import that dependency in the spec helper?

I added require 'pry' simply for the convenience of debugging, but I understand if that's superfluous.

jhollinger

Looks good, but I have some thoughts on the ordering of transformers.

jhollinger · 2024-01-02T19:47:19Z

lib/blueprinter/view_collection.rb

+      all_transformers = included_transformers.concat(views[:default].view_transformers).uniq
+      all_transformers.empty? ? Blueprinter.configuration.default_transformers : all_transformers


This is an edge case, but in what order should the transformers be applied? I would expect ones from the default view first, followed by included views, and finally any from the final view. Looks like this might apply the default view ones last. Thoughts?

Others have good questions will wait for those to be addressed.

m-graf

small comments

m-graf · 2024-01-08T17:20:21Z

spec/spec_helper.rb

+require 'pry'
+require 'json'


I hope not! :)

m-graf · 2024-01-08T17:21:35Z

spec/units/view_collection_spec.rb

+# frozen_string_literal: true
+
+describe 'ViewCollection' do
+  let(:view_collection) { Blueprinter::ViewCollection.new }


could we use subject here?

m-graf · 2024-01-08T17:21:48Z

spec/units/view_collection_spec.rb

+  let!(:default_view) { view_collection[:default] }
+  let!(:view) { view_collection[:view] }


Why are we using let! here?

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 view directly, so it won't be initialized unless we use let!.

Signed-off-by: Nate Baer <[email protected]>

jhollinger

👍

bhooshiek-narendiran and others added 4 commits November 24, 2023 00:33

Allowing to use included views transformer from the current view

2d3c9c7

Signed-off-by: bhooshiek <[email protected]>

Merge branch 'main' into nested_transformers

e3c8ecc

Improve logic for gathering transformers from included views.

8815272

Signed-off-by: Nate Baer <[email protected]>

Add view collection spec

dec674f

Signed-off-by: Nate Baer <[email protected]>

njbbaer self-assigned this Dec 26, 2023

Merge branch 'main' into nb/nested_transformers

1bcae06

njbbaer commented Dec 26, 2023

View reviewed changes

spec/units/view_collection_spec.rb Outdated Show resolved Hide resolved

njbbaer commented Dec 26, 2023

View reviewed changes

spec/units/view_collection_spec.rb Outdated Show resolved Hide resolved

njbbaer commented Dec 26, 2023

View reviewed changes

spec/units/view_collection_spec.rb Outdated Show resolved Hide resolved

- View transformers should combine with default transformers instead …

8e20388

…of replace them. - Enhance view collection test suite and address comments. Signed-off-by: Nate Baer <[email protected]>

njbbaer commented Dec 27, 2023

View reviewed changes

lib/blueprinter/view_collection.rb Outdated Show resolved Hide resolved

Fix handling of configured default_transformers, which is only used i…

0acf5d1

…f there are no view transformers. Signed-off-by: Nate Baer <[email protected]>

njbbaer commented Dec 27, 2023

View reviewed changes

Remove failing test for logic that was moved.

1ca67b2

Signed-off-by: Nate Baer <[email protected]>

njbbaer commented Dec 27, 2023

View reviewed changes

njbbaer changed the title ~~Allow transformers to be included by other views~~ Allow transformers to be included across views Dec 27, 2023

- Add documentation for including transformers across views.

c723f9e

- Minor textual changes to view collection spec. Signed-off-by: Nate Baer <[email protected]>

njbbaer marked this pull request as ready for review December 27, 2023 07:52

njbbaer requested review from ritikesh and a team as code owners December 27, 2023 07:52

njbbaer requested review from lessthanjacob and jhollinger December 27, 2023 08:27

jmeridth previously approved these changes Dec 27, 2023

View reviewed changes

lessthanjacob reviewed Dec 29, 2023

View reviewed changes

jhollinger reviewed Jan 2, 2024

View reviewed changes

m-graf reviewed Jan 8, 2024

View reviewed changes

Use subject instead of let in view collection spec.

04564e6

Signed-off-by: Nate Baer <[email protected]>

ritikesh mentioned this pull request Jan 9, 2024

Allowing to use included views transformer from the current view #373

Closed

3 tasks

Execute transformers in a top-down order.

fe71331

Signed-off-by: Nate Baer <[email protected]>

Remove pry import from spec helper.

23b167e

Signed-off-by: Nate Baer <[email protected]>

njbbaer requested review from jhollinger, m-graf, bhooshiek-narendiran, jmeridth and lessthanjacob January 9, 2024 19:53

m-graf approved these changes Jan 9, 2024

View reviewed changes

jhollinger approved these changes Jan 9, 2024

View reviewed changes

Merge branch 'main' into nb/nested_transformers

6d39a88

bhooshiek-narendiran approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into nb/nested_transformers

012a8dc

lessthanjacob approved these changes Jan 12, 2024

View reviewed changes

njbbaer merged commit 8b715cc into main Jan 12, 2024
12 checks passed

njbbaer deleted the nb/nested_transformers branch January 12, 2024 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow transformers to be included across views #372

Allow transformers to be included across views #372

njbbaer commented Dec 26, 2023 •

edited

Loading

njbbaer Dec 27, 2023 •

edited

Loading

jhollinger Jan 2, 2024

bhooshiek-narendiran Jan 7, 2024

bhooshiek-narendiran Jan 8, 2024

jhollinger Jan 8, 2024 •

edited

Loading

njbbaer Jan 9, 2024

bhooshiek-narendiran Jan 9, 2024

njbbaer Jan 9, 2024 •

edited

Loading

njbbaer Dec 27, 2023 •

edited

Loading

lessthanjacob Dec 29, 2023

njbbaer Jan 8, 2024

lessthanjacob Dec 29, 2023

m-graf Jan 8, 2024

njbbaer Jan 8, 2024 •

edited

Loading

jhollinger left a comment

jhollinger Jan 2, 2024

m-graf left a comment

m-graf Jan 8, 2024

m-graf Jan 8, 2024

m-graf Jan 8, 2024

njbbaer Jan 8, 2024

jhollinger left a comment

		all_transformers = included_transformers.concat(views[:default].view_transformers).uniq
		all_transformers.empty? ? Blueprinter.configuration.default_transformers : all_transformers

		let!(:default_view) { view_collection[:default] }
		let!(:view) { view_collection[:view] }

		require 'pry'
		require 'json'

		require 'pry'
		require 'json'

Allow transformers to be included across views #372

Allow transformers to be included across views #372

Conversation

njbbaer commented Dec 26, 2023 • edited Loading

njbbaer Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhollinger Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njbbaer Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

njbbaer Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njbbaer Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

jhollinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-graf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhollinger left a comment

Choose a reason for hiding this comment

njbbaer commented Dec 26, 2023 •

edited

Loading

njbbaer Dec 27, 2023 •

edited

Loading

jhollinger Jan 8, 2024 •

edited

Loading

njbbaer Jan 9, 2024 •

edited

Loading

njbbaer Dec 27, 2023 •

edited

Loading

njbbaer Jan 8, 2024 •

edited

Loading