-
Notifications
You must be signed in to change notification settings - Fork 151
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
support proper merging of nested conditionals #33
Conversation
This is great. Could you please update CHANGELOG, too? (Use |
(final[conditional_op] ||= {}).merge!(temp_step.delete(conditional_op)) | ||
end | ||
end | ||
final.merge!(temp_step) |
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 don't think you want to merge!
(with a bang) into the final result, inject
is supposed to return this, no?
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 dunno, it was like that when I got here :)
The bang isn't hurting anything, but you're right, it's not needed.
Questions? Comments? Cheap shots? |
options = (@block_options ||= []).inject({}){|final, step| final.merge!(step)}.merge(options) | ||
options = {} | ||
|
||
merge_logic = proc do |key, oldval, newval| |
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 creative, but a little unorthodox. You should just make this a private
method, something like merge_options
.
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.
Merge parameters aren't really old
or new
, they are more lhs
and rhs
(left hand side / right hand side), no?
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 suppose old
could be more appropriately named existing
as it is the value of the key in the existing hash accepting the merge, and new
is the value of the key in the new hash being merged in. That makes sense to my brain, but I guess lhs
and rhs
work just as well, since lhs
receives the merge--reminds me of an SQL JOIN
. I'll change it if you think lhs
and rhs
are more clear.
I made some minor comments. You also still need to update CHANGELOG, please. Thanks for being patient with my nitpicking. |
@dblock I updated CHANGELOG and moved all the merge logic into a private method. Let me know if you want to use |
Great, merging. |
support proper merging of nested conditionals
Currently, nested option blocks containing conditionals like this:
will produce an exposure that reflects only the innermost conditions:
To me, this is unexpected behavior. I would expect the following exposure from the code above:
This pull request enables proper merging of nested conditionals. All
Hash
conditions are merged into a single hash.Proc
andSymbol
conditions are pushed to a special "extras" array. For evaluation, the hash is added to the extras array and all the conditions are logically ANDed together.