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.

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

Add IncludeDirective#merge method #27

Closed
wants to merge 7 commits into from
Closed

Add IncludeDirective#merge method #27

Show file tree
Hide file tree
Changes from 5 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
31 changes: 31 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,29 @@ def [](key)
end
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.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

merged_options = merge_options(options, other.options)

IncludeDirective.new(hash, merged_options)
end

# @param another_directive [IncludeDirective]
# @return [IncludeDirective]
def merge!(other)
merge_result = 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.

As a general good practice, I tend to favor defining the "safe" method in terms of the "dangerous" method rather than the opposite.

@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 All @@ -63,5 +88,11 @@ def to_string

string_array.join(',')
end

private

def merge_options(opts, other_opts)
opts.merge(other_opts)
end
end
end
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