Add IncludeDirective#merge method #27
Conversation
Added IncludeDirective#merge and #merge! methods Added test to cover the new #merge method Added runtime_dependency to 'activesupport' (needed for Hash#deep_merge)
|
Completely missed it, it makes sense tough. |
| hash = to_hash.dup | ||
| Parser.deep_merge!(hash, other.to_hash) | ||
|
|
||
| IncludeDirective.new(hash, options.merge!(other.options)) |
There was a problem hiding this comment.
The semantics of options is not very clear during a merge.
There was a problem hiding this comment.
The problem here is that if one has two IncludeDirectives with conflicting options, it is not clear to the user how this conflict will be resolved. In this case it's the options of the other hash that will have priority.
There was a problem hiding this comment.
Ok I see your point. I took this decision thinking about allow_wildcard (which looks to me like the only available option at the moment). But thinking again about it, it definitely doesn't work well.
Currently you only need to specify allow_wildcard if you have wildcards in your include.
If this is not the case for other, than whatever is in self remain. If you need it in your new hash, instead, that will be merged properly in the result.
The only case this is not working as everyone would expect, is when other.options explicitly contains allow_wildcard: false.
There was a problem hiding this comment.
Exactly. I think the proper way of handling that would be to have a dedicated method to merge the options.
There was a problem hiding this comment.
Agreed, can do that.
Any advice on the logic of it? Can I still do options.merge!(other.options) ?
There was a problem hiding this comment.
Ok method is ready, just need a strategy for it, or I can leave the current implementation of options.merge!(other.options) so that the options of the other hash that will have priority.
|
A few nitpicking remarks, but looks good! |
|
No problem, will try to address all of them 😄 |
|
Hi @beauby, just touching base to know if there's anything blocking this at the moment 😄 |
|
@beauby I'm sorry for chasing you, this really is critical for me. |
|
Sorry, I've been a bit busy. Will review it tonight! On Thursday, 21 July 2016, Mattia notifications@github.com wrote:
Lucas Hosseini |
|
Thanks @beauby, no worries I understand 😃 |
| def merge!(other) | ||
| merge_result = merge(other) | ||
| @hash = merge_result.hash | ||
| @options = merge_result.options |
There was a problem hiding this comment.
After giving a thought about the options merging thing, I believe the most sane thing to do here is to assume that when one merges an IncludeDirective B into an other IncludeDirective A, we keep A's options.
|
Reviewed the code. However, in order to merge this PR into this gem, I think we would need a JSON API use-case, as this gem is aimed towards JSON API. AMS#1333 goes against the spec (which specifies that when a client explicitly requires associations via |
… discarded, no more options merge refactoring of #merge and #merge! (safe method defined in terms of the dangerous one) removed useless #dup call on to_hash result since to_hash already returns a new object
|
@beauby pushed an updated version after going through your review. Regarding the JSON API use case, I understand the situation. That said, the only generic use-case I can imagine for the merge method is when you want to define them on your application logic layer and you need to work with inheritance "extending" your superclass include directive. Hope that makes sense. If it doesn't and you think this is far from being a useful addition to JSONAPI gem, I can always add it as an extension in AMS 😃 |
|
As there does not seem to be a JSON API specific need for this feature, I'm going to close this PR and suggest taking it up with AMS. Great work nonetheless! |
Added IncludeDirective#merge and #merge! methods
Added runtime_dependency to 'activesupport' (needed for Hash#deep_merge)
All covered with new tests 😄