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

support proper merging of nested conditionals #33

Merged
merged 1 commit into from
Nov 4, 2013
Merged

support proper merging of nested conditionals #33

merged 1 commit into from
Nov 4, 2013

Conversation

wyattisimo
Copy link
Contributor

Currently, nested option blocks containing conditionals like this:

with_options(:if => {:awesome => true}) do
  with_options(:if => {:less_awesome => false}) do
    expose :awesome_thing
  end
end

will produce an exposure that reflects only the innermost conditions:

{:if => {:less_awesome => false}}

To me, this is unexpected behavior. I would expect the following exposure from the code above:

{:if => {:awesome => true, :less_awesome => false}}

This pull request enables proper merging of nested conditionals. All Hash conditions are merged into a single hash. Proc and Symbol 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.

@dblock
Copy link
Member

dblock commented Nov 2, 2013

This is great. Could you please update CHANGELOG, too? (Use git --amend).

(final[conditional_op] ||= {}).merge!(temp_step.delete(conditional_op))
end
end
final.merge!(temp_step)
Copy link
Member

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?

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 dunno, it was like that when I got here :)

The bang isn't hurting anything, but you're right, it's not needed.

@wyattisimo
Copy link
Contributor Author

Questions? Comments? Cheap shots?

options = (@block_options ||= []).inject({}){|final, step| final.merge!(step)}.merge(options)
options = {}

merge_logic = proc do |key, oldval, newval|
Copy link
Member

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.

Copy link
Member

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?

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

@dblock
Copy link
Member

dblock commented Nov 3, 2013

I made some minor comments. You also still need to update CHANGELOG, please.

Thanks for being patient with my nitpicking.

@wyattisimo
Copy link
Contributor Author

@dblock I updated CHANGELOG and moved all the merge logic into a private method. Let me know if you want to use lhs and rhs in that merge proc.

@dblock
Copy link
Member

dblock commented Nov 4, 2013

Great, merging.

dblock added a commit that referenced this pull request Nov 4, 2013
support proper merging of nested conditionals
@dblock dblock merged commit 50e5c5d into ruby-grape:master Nov 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants