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.

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

Support keyword arguments on all Concurrent Struct classes #738

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/concurrent/immutable_struct.rb
Expand Up @@ -71,20 +71,20 @@ def select(&block)
end

# @!macro struct_new
def self.new(*args, &block)
def self.new(*args, **kw_args, &block)
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to have the signature more specific def self.new(*args, keyword_init: false, &block). Afaik no other keyword argument has any effect.

clazz_name = nil
if args.length == 0
raise ArgumentError.new('wrong number of arguments (0 for 1+)')
elsif args.length > 0 && args.first.is_a?(String)
clazz_name = args.shift
end
FACTORY.define_struct(clazz_name, args, &block)
FACTORY.define_struct(clazz_name, args, kw_args, &block)
end

FACTORY = Class.new(Synchronization::LockableObject) do
def define_struct(name, members, &block)
def define_struct(name, members, kw_args, &block)
synchronize do
Synchronization::AbstractStruct.define_struct_class(ImmutableStruct, Synchronization::Object, name, members, &block)
Synchronization::AbstractStruct.define_struct_class(ImmutableStruct, Synchronization::Object, name, members, kw_args, &block)
end
end
end.new
Expand Down
8 changes: 4 additions & 4 deletions lib/concurrent/mutable_struct.rb
Expand Up @@ -197,20 +197,20 @@ def []=(member, value)
end

# @!macro struct_new
Copy link
Member

Choose a reason for hiding this comment

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

Please update the struct_new definition to include documentation about keyword_init option.

def self.new(*args, &block)
def self.new(*args, **kw_args, &block)
clazz_name = nil
if args.length == 0
raise ArgumentError.new('wrong number of arguments (0 for 1+)')
elsif args.length > 0 && args.first.is_a?(String)
clazz_name = args.shift
end
FACTORY.define_struct(clazz_name, args, &block)
FACTORY.define_struct(clazz_name, args, kw_args, &block)
end

FACTORY = Class.new(Synchronization::LockableObject) do
def define_struct(name, members, &block)
def define_struct(name, members, kw_args, &block)
synchronize do
clazz = Synchronization::AbstractStruct.define_struct_class(MutableStruct, Synchronization::LockableObject, name, members, &block)
clazz = Synchronization::AbstractStruct.define_struct_class(MutableStruct, Synchronization::LockableObject, name, members, kw_args, &block)
members.each_with_index do |member, index|
clazz.send :remove_method, member
clazz.send(:define_method, member) do
Expand Down
8 changes: 4 additions & 4 deletions lib/concurrent/settable_struct.rb
Expand Up @@ -92,20 +92,20 @@ def []=(member, value)
end

# @!macro struct_new
def self.new(*args, &block)
def self.new(*args, **kw_args, &block)
clazz_name = nil
if args.length == 0
raise ArgumentError.new('wrong number of arguments (0 for 1+)')
elsif args.length > 0 && args.first.is_a?(String)
clazz_name = args.shift
end
FACTORY.define_struct(clazz_name, args, &block)
FACTORY.define_struct(clazz_name, args, kw_args, &block)
end

FACTORY = Class.new(Synchronization::LockableObject) do
def define_struct(name, members, &block)
def define_struct(name, members, kw_args, &block)
synchronize do
clazz = Synchronization::AbstractStruct.define_struct_class(SettableStruct, Synchronization::LockableObject, name, members, &block)
clazz = Synchronization::AbstractStruct.define_struct_class(SettableStruct, Synchronization::LockableObject, name, members, kw_args, &block)
members.each_with_index do |member, index|
clazz.send :remove_method, member if clazz.instance_methods.include? member
clazz.send(:define_method, member) do
Expand Down
28 changes: 22 additions & 6 deletions lib/concurrent/synchronization/abstract_struct.rb
Expand Up @@ -6,9 +6,18 @@ module Synchronization
module AbstractStruct

# @!visibility private
def initialize(*values)
def initialize(*values, **kw_values)
super()
ns_initialize(*values)
ns_initialize(*values, **kw_values)
end

# @!macro [attach] struct_keyword_init
Copy link
Member

Choose a reason for hiding this comment

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

The macro definition at the beginning of the can doc can be omitted, if the doc is not shared with other methods.

#
# Returns `true` if the struct uses keyword arguments.
#
# @return [Boolean] true if struct uses keyword arguments
def keyword_init?
self.class::KEYWORD_INIT
end
Copy link
Member

Choose a reason for hiding this comment

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

The keyword_init? on struct instance probably has not usage other then the initialization, I think it can be omitted or moved to the struct class replacing the KEYWORD_INIT constant.


# @!macro [attach] struct_length
Expand Down Expand Up @@ -127,13 +136,20 @@ def pr_underscore(clazz)
end

# @!visibility private
def self.define_struct_class(parent, base, name, members, &block)
def self.define_struct_class(parent, base, name, members, kw_args, &block)
clazz = Class.new(base || Object) do
include parent
self.const_set(:MEMBERS, members.collect{|member| member.to_s.to_sym}.freeze)
def ns_initialize(*values)
raise ArgumentError.new('struct size differs') if values.length > length
@values = values.fill(nil, values.length..length-1)
self.const_set(:KEYWORD_INIT, !!kw_args[:keyword_init])
def ns_initialize(*values, **kw_values)
@values = if keyword_init?
key_diff = kw_values.keys - members
raise ArgumentError.new("unknown keywords: #{key_diff.join(',')}") unless key_diff.empty?
members.map {|val| kw_values.fetch(val, nil)}
else
raise ArgumentError.new('struct size differs') if values.length > length
values.fill(nil, values.length..length-1)
end
end
end
unless name.nil?
Expand Down
37 changes: 32 additions & 5 deletions spec/concurrent/struct_shared.rb
Expand Up @@ -27,7 +27,8 @@
members = [:Foo, :bar, 'baz']
structs = [
described_class.new(*members).new,
described_class.new('ClassForCheckingGetterDefinition', *members).new
described_class.new(*members, keyword_init: true).new,
described_class.new('ClassForCheckingGetterDefinition', *members).new,
]

structs.each do |struct|
Expand All @@ -54,8 +55,11 @@ def baz(foo, bar) foo + bar; end
clazz2 = described_class.new(:foo, :bar) do
def baz(foo, bar) foo + bar; end
end
clazz3 = described_class.new(:foo, :bar, keyword_init: true) do
def baz(foo, bar) foo + bar; end
end

[clazz1, clazz2].each do |clazz|
[clazz1, clazz2, clazz3].each do |clazz|
struct = clazz.new
expect(struct).to respond_to :baz
expect(struct.method(:baz).arity).to eq 2
Expand All @@ -68,9 +72,11 @@ def baz(foo, bar) foo + bar; end

let!(:members){ [:Foo, :bar, 'baz'] }
let!(:values){ [42, '42', :fortytwo] }
let!(:kw_values){ {Foo: 42, bar: '42', baz: :fortytwo} }
let!(:classes) do
[
described_class.new(*members),
described_class.new(*members, keyword_init: true),
described_class.new('StructConstructionTester', *members)
]
end
Expand All @@ -86,7 +92,7 @@ def baz(foo, bar) foo + bar; end

it 'sets all given members in order' do
classes.each do |clazz|
struct = clazz.new(*values)
struct = clazz::KEYWORD_INIT ? clazz.new(**kw_values) : clazz.new(*values)
members.each_with_index do |member, index|
expect(struct.send(member)).to eq values[index]
end
Expand All @@ -95,8 +101,13 @@ def baz(foo, bar) foo + bar; end

it 'raises an exception when extra members are given' do
classes.each do |clazz|
extra_values = values << 'forty two'
expect{ clazz.new(*extra_values) }.to raise_error(ArgumentError)
if clazz::KEYWORD_INIT
extra_values = kw_values.merge({FortyTwo: 'forty two'})
expect{ clazz.new(**extra_values) }.to raise_error(ArgumentError)
else
extra_values = values << 'forty two'
expect{ clazz.new(*extra_values) }.to raise_error(ArgumentError)
end
end
end
end
Expand All @@ -105,6 +116,7 @@ def baz(foo, bar) foo + bar; end

let!(:anon_struct_members) { [:name, :address, :zip] }
let(:anon_struct) { described_class.new(*anon_struct_members) }
let(:anon_struct_with_keyword_init) { described_class.new(*anon_struct_members, keyword_init: true) }

let!(:named_struct_members) { [:left, :right] }
let(:named_struct) do
Expand All @@ -116,19 +128,31 @@ def baz(foo, bar) foo + bar; end

it 'returns the number of struct members' do
expect(anon_struct.new.length).to eq anon_struct_members.length
expect(anon_struct_with_keyword_init.new.length).to eq anon_struct_members.length
expect(named_struct.new.length).to eq named_struct_members.length
end
end

context '#keyword_init?' do

it 'returns a boolean indicating if keywords are enabled for initialization' do
expect(anon_struct.new.keyword_init?).to eq false
expect(anon_struct_with_keyword_init.new.keyword_init?).to eq true
expect(named_struct.new.keyword_init?).to eq false
end
end

context '#members' do

it 'returns the struct members as an array of symbols' do
expect(anon_struct.new.members).to eq anon_struct_members
expect(anon_struct_with_keyword_init.new.members).to eq anon_struct_members
expect(named_struct.new.members).to eq named_struct_members
end

it 'returns a different object than the array passed at definition' do
expect(anon_struct.new.members.object_id).to_not eq anon_struct_members.object_id
expect(anon_struct_with_keyword_init.new.members.object_id).to_not eq anon_struct_members.object_id
expect(named_struct.new.members.object_id).to_not eq named_struct_members.object_id
end
end
Expand All @@ -137,6 +161,7 @@ def baz(foo, bar) foo + bar; end

it 'returns the number of struct members' do
expect(anon_struct.new.size).to eq anon_struct_members.size
expect(anon_struct_with_keyword_init.new.size).to eq anon_struct_members.size
expect(named_struct.new.size).to eq named_struct_members.size
end
end
Expand All @@ -145,9 +170,11 @@ def baz(foo, bar) foo + bar; end

it 'returns the values of the struct as an array in order' do
expect(anon_struct.new().values).to eq [nil, nil, nil]
expect(anon_struct_with_keyword_init.new().values).to eq [nil, nil, nil]
expect(named_struct.new().values).to eq [nil, nil]

expect(anon_struct.new(:foo, :bar, :baz).values).to eq [:foo, :bar, :baz]
expect(anon_struct_with_keyword_init.new(name: :foo, address: :bar, zip: :baz).values).to eq [:foo, :bar, :baz]
expect(named_struct.new(:yes, :no).values).to eq [:yes, :no]
end
end
Expand Down