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 all 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)
dup.merge!(other)
end

# @param another_directive [IncludeDirective]
# @return [IncludeDirective]
def merge!(other)
fail ArgumentError,
"parameter MUST be an IncludeDirective" unless
other.is_a?(IncludeDirective)

hash = to_hash
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


merge_result = IncludeDirective.new(hash, options)
@hash = merge_result.hash
@options = merge_result.options
Copy link
Owner

Choose a reason for hiding this comment

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

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.

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