Skip to content

Commit

Permalink
drop Typelike SAFE_MODIFIED_COPY_METHODS. implement Hashlike#select, …
Browse files Browse the repository at this point in the history
…#reject, #compact, #merge. implement Arraylike #select, #reject, #compact. no longer implement #transform_values to return a modified copy.

this fixes an issue where Hashlike#select and #reject returned a modified copy (a SchemaInstanceBase or JSON::HashNode) but the block variables were from the underlying hash, instead of the receiver.

transform_values isn't practical to return a modified copy because putting block result into the typelike object is ... weird.
  • Loading branch information
notEthan committed Jun 12, 2018
1 parent 75fe96a commit f1421ea
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 39 deletions.
18 changes: 0 additions & 18 deletions lib/scorpio/json/node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -214,15 +214,6 @@ def as_json # needs redefined after including Enumerable
SAFE_INDEX_ONLY_METHODS.each do |method_name|
define_method(method_name) { |*a, &b| content.public_send(method_name, *a, &b) }
end

# methods that return a modified copy
SAFE_MODIFIED_COPY_METHODS.each do |method_name|
define_method(method_name) do |*a, &b|
modified_copy do |content_to_modify|
content_to_modify.public_send(method_name, *a, &b)
end
end
end
end

class HashNode < Node
Expand Down Expand Up @@ -250,15 +241,6 @@ def as_json # needs redefined after including Enumerable
SAFE_KEY_ONLY_METHODS.each do |method_name|
define_method(method_name) { |*a, &b| content.public_send(method_name, *a, &b) }
end

# methods that return a modified copy
SAFE_MODIFIED_COPY_METHODS.each do |method_name|
define_method(method_name) do |*a, &b|
modified_copy do |content_to_modify|
content_to_modify.public_send(method_name, *a, &b)
end
end
end
end
end
end
16 changes: 0 additions & 16 deletions lib/scorpio/schema_instance_base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,6 @@ def to_hash
define_method(method_name) { |*a, &b| instance.public_send(method_name, *a, &b) }
end

SAFE_MODIFIED_COPY_METHODS.each do |method_name|
define_method(method_name) do |*a, &b|
modified_copy do |instance_to_modify|
instance_to_modify.public_send(method_name, *a, &b)
end
end
end

def [](property_name_)
@instance_mapped ||= Hash.new do |hash, property_name|
hash[property_name] = begin
Expand Down Expand Up @@ -282,14 +274,6 @@ def to_ary
define_method(method_name) { |*a, &b| instance.public_send(method_name, *a, &b) }
end

SAFE_MODIFIED_COPY_METHODS.each do |method_name|
define_method(method_name) do |*a, &b|
modified_copy do |instance_to_modify|
instance_to_modify.public_send(method_name, *a, &b)
end
end
end

def [](i_)
# it would make more sense for this to be an array here, but but Array doesn't have a nice memoizing
# constructor, so it's a hash with integer keys
Expand Down
38 changes: 35 additions & 3 deletions lib/scorpio/typelike_modules.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,26 @@ module Hashlike
DESTRUCTIVE_METHODS = %w(clear delete delete_if keep_if reject! replace select! shift)
# methods which return a modified copy, which you'd expect to be of the same class as the receiver.
# there are some ambiguous ones that are omitted, like #invert.
SAFE_MODIFIED_COPY_METHODS = %w(compact merge reject select transform_values)
SAFE_METHODS = SAFE_KEY_ONLY_METHODS | SAFE_KEY_VALUE_METHODS
SAFE_METHODS.each do |method_name|
define_method(method_name) { |*a, &b| to_hash.public_send(method_name, *a, &b) }
end
%w(compact merge).each do |method_name|
define_method(method_name) do |*a, &b|
Scorpio::Typelike.modified_copy(self) do |object_to_modify|
object_to_modify.public_send(method_name, *a, &b)
end
end
end
%w(select reject).each do |method_name|
define_method(method_name) do |*a, &b|
Scorpio::Typelike.modified_copy(self) do |object_to_modify|
object_to_modify.public_send(method_name, *a) do |k, _v|
b.call(k, self[k])
end
end
end
end

def inspect
object_group_text = respond_to?(:object_group_text) ? ' ' + self.object_group_text : ''
Expand Down Expand Up @@ -91,13 +106,30 @@ module Arraylike
# there are some ambiguous ones that are omitted, like #sort, #map / #collect.
SAFE_INDEX_ELEMENT_METHODS = %w(| & * + - <=> abbrev assoc at bsearch bsearch_index combination compact count cycle dig drop drop_while fetch find_index first include? index join last pack permutation product reject repeated_combination repeated_permutation reverse reverse_each rindex rotate sample select shelljoin shuffle slice sort take take_while transpose uniq values_at zip)
DESTRUCTIVE_METHODS = %w(<< clear collect! compact! concat delete delete_at delete_if fill flatten! insert keep_if map! pop push reject! replace reverse! rotate! select! shift shuffle! slice! sort! sort_by! uniq! unshift)
# methods which return a modified copy, which you'd expect to be of the same class as the receiver.
SAFE_MODIFIED_COPY_METHODS = %w(compact reject select)

SAFE_METHODS = SAFE_INDEX_ONLY_METHODS | SAFE_INDEX_ELEMENT_METHODS
SAFE_METHODS.each do |method_name|
define_method(method_name) { |*a, &b| to_ary.public_send(method_name, *a, &b) }
end
# methods (well, method) that returns a modified copy and doesn't need any handling of block variable(s)
%w(compact).each do |method_name|
define_method(method_name) do |*a, &b|
Scorpio::Typelike.modified_copy(self) do |object_to_modify|
object_to_modify.public_send(method_name, *a, &b)
end
end
end
# methods that return a modified copy and do need handling of block variables
%w(reject select).each do |method_name|
define_method(method_name) do |*a, &b|
Scorpio::Typelike.modified_copy(self) do |object_to_modify|
i = 0
object_to_modify.public_send(method_name, *a) do |_e|
b.call(self[i]).tap { i += 1 }
end
end
end
end

def inspect
object_group_text = respond_to?(:object_group_text) ? ' ' + self.object_group_text : ''
Expand Down
8 changes: 8 additions & 0 deletions test/schema_instance_base_array_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,15 @@
end
describe 'modified copy methods' do
it('#reject') { assert_equal(class_for_schema.new(Scorpio::JSON::ArrayNode.new(['foo'], [])), subject.reject { |e| e != 'foo' }) }
it('#reject block var') do
subj_a = subject.to_a
subject.reject { |e| assert_equal(e, subj_a.shift) }
end
it('#select') { assert_equal(class_for_schema.new(Scorpio::JSON::ArrayNode.new(['foo'], [])), subject.select { |e| e == 'foo' }) }
it('#select block var') do
subj_a = subject.to_a
subject.select { |e| assert_equal(e, subj_a.shift) }
end
it('#compact') { assert_equal(subject, subject.compact) }
describe 'at a depth' do
let(:document) { [['b', 'q'], {'c' => ['d', 'e']}] }
Expand Down
9 changes: 8 additions & 1 deletion test/schema_instance_base_hash_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
it('#rassoc') { assert_equal(['baz', true], subject.rassoc(true)) }
it('#to_h') { assert_equal({'foo' => subject['foo'], 'bar' => subject['bar'], 'baz' => true}, subject.to_h) }
it('#to_proc') { assert_equal(true, subject.to_proc.call('baz')) } if {}.respond_to?(:to_proc)
it('#transform_values') { assert_equal({'foo' => nil, 'bar' => nil, 'baz' => nil}, subject.transform_values { |_| nil}) }
it('#value?') { assert_equal(false, subject.value?('0')) }
it('#values') { assert_equal([subject['foo'], subject['bar'], true], subject.values) }
it('#values_at') { assert_equal([true], subject.values_at('baz')) }
Expand All @@ -84,9 +85,15 @@
# I'm going to rely on the #merge test above to test the modified copy functionality and just do basic
# tests of all the modified copy methods here
it('#merge') { assert_equal(subject, subject.merge({})) }
it('#transform_values') { assert_equal(class_for_schema.new(Scorpio::JSON::HashNode.new({'foo' => nil, 'bar' => nil, 'baz' => nil}, [])), subject.transform_values { |_| nil}) }
it('#reject') { assert_equal(class_for_schema.new(Scorpio::JSON::HashNode.new({}, [])), subject.reject { true }) }
it('#select') { assert_equal(class_for_schema.new(Scorpio::JSON::HashNode.new({}, [])), subject.select { false }) }
describe '#select' do
it 'yields properly too' do
subject.select do |k, v|
assert_equal(subject[k], v)
end
end
end
# Hash#compact only available as of ruby 2.5.0
if {}.respond_to?(:compact)
it('#compact') { assert_equal(subject, subject.compact) }
Expand Down
2 changes: 1 addition & 1 deletion test/scorpio_json_hashnode_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
it('#rassoc') { assert_equal(['a', 'b'], node.rassoc('b')) }
it('#to_h') { assert_equal({'a' => 'b', 'c' => node['c']}, node.to_h) }
it('#to_proc') { assert_equal('b', node.to_proc.call('a')) } if {}.respond_to?(:to_proc)
it('#transform_values') { assert_equal({'a' => nil, 'c' => nil}, node.transform_values { |_| nil}) }
it('#value?') { assert_equal(false, node.value?('0')) }
it('#values') { assert_equal(['b', node['c']], node.values) }
it('#values_at') { assert_equal(['b'], node.values_at('a')) }
Expand All @@ -98,7 +99,6 @@
# I'm going to rely on the #merge test above to test the modified copy functionality and just do basic
# tests of all the modified copy methods here
it('#merge') { assert_equal(node, node.merge({})) }
it('#transform_values') { assert_equal(Scorpio::JSON::Node.new_by_type({'a' => nil, 'c' => nil}, []), node.transform_values { |_| nil}) }
it('#reject') { assert_equal(Scorpio::JSON::Node.new_by_type({}, []), node.reject { true }) }
it('#select') { assert_equal(Scorpio::JSON::Node.new_by_type({}, []), node.select { false }) }
# Hash#compact only available as of ruby 2.5.0
Expand Down

0 comments on commit f1421ea

Please sign in to comment.