explicitly track field definition order when used to sort output fields #197
Conversation
|
sweet |
|
|
||
| def initialize(name, fields: {}, included_view_names: [], excluded_view_names: [],transformers: []) | ||
| @name = name | ||
| @fields = fields | ||
| @included_view_names = included_view_names | ||
| @excluded_field_names = excluded_view_names | ||
| @transformers = transformers | ||
| @definition_order = [] |
There was a problem hiding this comment.
so @definition_order is an Array of either strings and/or hashes, where strings represents field_names and hashes represents view_name right? That doesn't seem to be very intuitive, do you think there's another way to map the ordering that would be more explicit?
There was a problem hiding this comment.
Structs with a name and a type (:field or :view) each? The main thing is to avoid the possibility of name collisions between views and fields without hacky magic substrings in the array like "actually_its_a_view_#{view_name}" . include_view support means we can't just stick the view object itself in there.
There was a problem hiding this comment.
As the last push it is an array of DefinitionPlaceholder instances.
lib/blueprinter/view_collection.rb
Outdated
| included_fields.merge(source_fields) | ||
| def sort_by_def(view_name, fields) | ||
| result = views[:default].definition_order.reduce({}) do |memo, key| | ||
| if key.is_a?(Hash) |
There was a problem hiding this comment.
it's very implicit right here that a Hash key is actually a view.
There was a problem hiding this comment.
So with Structs it would have a case statement on the type here and likewise in the renamed reduce_helper.
lib/blueprinter/view_collection.rb
Outdated
| def sort_by_def(view_name, fields) | ||
| result = views[:default].definition_order.reduce({}) do |memo, key| | ||
| if key.is_a?(Hash) | ||
| reduce_helper(memo, key, fields) if view_name == key.keys.first # recur all the way down on the given view_name but no others! |
There was a problem hiding this comment.
Can we rename reduce_helper to something else that is a little more descriptive?
There was a problem hiding this comment.
add_to_ordered_fields is the first thing to come to mind, with ordered_fields naming the first argument.
lib/blueprinter/view.rb
Outdated
| @@ -1,5 +1,6 @@ | |||
| module Blueprinter | |||
| # @api private | |||
| DefinitionPlaceholder = Struct.new :name, :for_a_view? | |||
There was a problem hiding this comment.
instead of for_a_view?, view? would be more ruby like
|
@philipqnguyen I've had a thought lately - how worried are you about cycles in the notional view inclusion graph? I'm not sure if that's been supported by the previous implementation or not. I've taken no measures to prevent or fail gracefully and it seems like it could lead to infinite recursion now. It'll be quick to add a test to see what happens but I haven't had thoughts about a solution yet. |
|
On a closer look, the infinite recursion happens in |
|
@wlkrw I'll give this another review in a few days! Sorry just been a little busy! |
There was a problem hiding this comment.
@wlkrw The infinite recursion from cyclical views was already happening previously on the master branch. It's not introduced here. I also don't see any reason to save people from getting themselves into this cyclical hole. This looks good to me.
| end | ||
| end | ||
|
|
||
| def merge_fields(source_fields, included_fields) | ||
| source_fields.merge! included_fields |
There was a problem hiding this comment.
Any reason we wanted to make this merge! vs merge? If we stick with merge! we should probably change this method name to merge_fields! and there is no longer a need to do: fields = merge_fields!(fields... but I think I'd prefer this method without side-effects.
There was a problem hiding this comment.
This comment is notable. I think we can refactor it after, for now I think we can go ahead and merge this
|
One minor comment otherwise it looks good to me! |
This is a fix for #172. Thanks to #185 I saw that the problem comes from how fields are merged at render time. Hash insertion order was not quite enough but probably could have been made to work with some more bookkeeping during the field definition which would then be taken account of in
ViewCollection#sortable_fieldsand#merge_fieldsRather than increasing the amount of fancy footwork involved with merging hashes, each
Viewnow builds up an ordering at definition time which is traversed at render time.One edge case to note, is that if for some reason a
Viewis defined over multiple block yields, the output order will be as if all those fields were added inside the first one. This seems no worse than the current situation where all those fields would be grouped together in an incorrect location.