Skip to content

Add IncludeDirective#merge method #27

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Add IncludeDirective#merge method #27

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
37daf54
Added attr_readers for IncludeDirective #hash and #options
iMacTia Jul 13, 2016
12e5dc4
improved tests to cover new #merge! method as well
iMacTia Jul 13, 2016
20b99e3
removed dependency with activesupport and using Parser#deep_merge! in…
iMacTia Jul 14, 2016
cd113f5
minor syntax fixes
iMacTia Jul 14, 2016
59ed259
added separate #merge_options method to make it clearer
iMacTia Jul 15, 2016
092abaf
as a result of A.merge(B), options of A will be used and options of B…
iMacTia Jul 22, 2016
9eba292
fixed typo
iMacTia Jul 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions jsonapi_parser.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ Gem::Specification.new do |spec|
spec.test_files = spec.files.grep(%r{^(spec)/})
spec.require_paths = ['lib']

spec.add_dependency 'json', '~>1.8'
spec.add_dependency 'json', '~> 1.8'

spec.add_development_dependency 'rake', '>=0.9'
spec.add_development_dependency 'rspec', '~>3.4'
spec.add_development_dependency 'rake', '>= 0.9'
spec.add_development_dependency 'rspec', '~> 3.4'
spec.add_development_dependency 'simplecov'
end
24 changes: 24 additions & 0 deletions lib/jsonapi/include_directive.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ module JSONAPI
# @example 'posts.**' # => Include related posts, and all the included
# posts' related resources, and their related resources, recursively.
class IncludeDirective
attr_reader :options, :hash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hash should not have a reader - use the to_hash method or @hash instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately I need it in #merge! to do merge_result.hash (just renamed from new)

Copy link
Owner

@beauby beauby Jul 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of exposing the hash, you could just make IncludeDirective enumerable by

include Enumerable

def each
  @hash.each
end

then

def merge!(other)
  other.each do |key, val|
    @hash[key] ||= IncludeDirective.new({}, options)
    @hash[key].merge!(val)
  end
end

def merge(other)
  id = IncludeDirective.new(to_hash, options)
  id.merge!(other)

  id
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also JSONAPI::IncludeDirective#hash overrides Object#hash method that is used for equality matching.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the advices!
I will only be able to add more commits from the 20th of September as I'm on holiday at the moment 😅


# @param include_args (see Parser.include_hash_from_include_args)
def initialize(include_args, options = {})
include_hash = Parser.parse_include_args(include_args)
Expand Down Expand Up @@ -40,6 +42,28 @@ def [](key)
end
end

# @param another_directive [IncludeDirective]
# @return [IncludeDirective]
def merge(other)
fail ArgumentError,
"the value of 'other' MUST be an IncludeDirective" unless
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the error message should reference the parameter name as it would not be clear out of context.

other.is_a?(IncludeDirective)

hash = to_hash.dup
Parser.deep_merge!(hash, other.to_hash)
Copy link
Owner

@beauby beauby Jul 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem if you do that is that when A = {}, B = { a: {} }, when you do A.merge!(B) and subsequently modify B, A[:a] will be modified as well. I can't really think of a use case where one would do modifications, but it might be confusing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually storing the result after duplicating it with to_hash.dup to specifically avoid this issue 😃
However, after a careful read I noticed that #to_hash should already return a copy, not the original reference. So probably calling #dup on it is not really needed


IncludeDirective.new(hash, options.merge!(other.options))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics of options is not very clear during a merge.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I make it clearer 😄?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. I think the proper way of handling that would be to have a dedicated method to merge the options.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, can do that.
Any advice on the logic of it? Can I still do options.merge!(other.options) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

# @param another_directive [IncludeDirective]
# @return [IncludeDirective]
def merge!(other)
new = merge(other)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd stay clear from new as a local variable name as it throws off syntax highlighting, and might confuse readers.

@hash = new.hash
@options = new.options
self
end

# @return [Hash{Symbol => Hash}]
def to_hash
@hash.each_with_object({}) do |(key, value), hash|
Expand Down
41 changes: 41 additions & 0 deletions spec/merge_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
require 'jsonapi/include_directive'

describe JSONAPI::IncludeDirective, '.merge' do
it 'works' do
d1 = JSONAPI::IncludeDirective.new(
{
post: {
comments: {
references: [:url]
},
author: [:address]
}
}
)
d2 = JSONAPI::IncludeDirective.new(
{
post: {
comments: [:length],
author: {},
created_at: {}
}
}
)
expected = JSONAPI::IncludeDirective.new(
{
post: {
comments: {
references: [:url],
length: {}
},
author: [:address],
created_at: {}
}
}
).to_hash

expect(d1.merge(d2).to_hash).to eq(expected)
d1.merge!(d2)
expect(d1.to_hash).to eq(expected)
end
end