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

Deprecate constructor as a block #110

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion docsite/source/index.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class App
end
# Defaults to nil if no default value is given
setting :adapter
# Pre-process values
# Construct values
setting(:path, 'test') { |value| Pathname(value) }
# Passing the reader option as true will create attr_reader method for the class
setting :pool, 5, reader: true
Expand Down
6 changes: 6 additions & 0 deletions lib/dry/configurable/dsl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require 'dry/configurable/settings'
require 'dry/configurable/compiler'
require 'dry/configurable/dsl/args'
require 'dry/core/deprecations'

module Dry
module Configurable
Expand Down Expand Up @@ -49,6 +50,11 @@ def setting(name, *args, &block)
if block.arity.zero?
ast << [:nested, [node, DSL.new(&block).ast]]
else
Dry::Core::Deprecations.announce(
'passing a constructor as a block',
'Provide a `constructor:` keyword argument instead',
tag: 'dry-configurable'
)
ast << [:constructor, [node, block]]
end
else
Expand Down
22 changes: 10 additions & 12 deletions spec/integration/dry/configurable/setting_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@
end
end

context 'with a value pre-processor' do
it 'pre-processes the value with nil default' do
klass.setting(:path, nil) { |value| "test:#{value || "fallback"}" }
context 'with a value constructor' do
it 'constructs the value with nil default' do
klass.setting(:path, nil, constructor: ->(value) { "test:#{value || "fallback"}" })

expect(object.config.path).to eql("test:fallback")

Expand All @@ -110,22 +110,20 @@
expect(object.config.path).to eql('test:foo')
end

it 'pre-processes the value with undefined default' do
klass.setting(:path) { |value| "test:#{value || "fallback"}" }
it 'constructs the value with undefined default' do
klass.setting(:path, constructor: ->(value) { "test:#{value || "fallback"}" })

expect(object.config.path).to eql('test:fallback')
end

it 'pre-processes the value with non-nil default' do
klass.setting(:path, 'test') { |value| Pathname(value) }
it 'constructs the value with non-nil default' do
klass.setting(:path, 'test', constructor: ->(value) { Pathname(value) })

expect(object.config.path).to eql(Pathname('test'))
end

it 'raises pre-processor errors immediately' do
klass.setting :failable do |value|
value.to_sym unless value.nil?
end
it 'raises constructor errors immediately' do
klass.setting(:failable, constructor: ->(value) { value.to_sym unless value.nil? })

expect {
object.config.failable = 12
Expand Down Expand Up @@ -316,7 +314,7 @@
end

it 'creates distinct setting values across instances' do
klass.setting(:path, 'test') { |m| Pathname(m) }
klass.setting(:path, 'test', constructor: ->(m) { Pathname(m) })

new_object = klass.new

Expand Down
9 changes: 9 additions & 0 deletions spec/unit/dry/configurable/dsl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@
end

it 'compiles a setting with a constructor' do
setting = dsl.setting(:dsn, 'sqlite', constructor: ->(value) { "jdbc:#{value}" })

expect(setting.name).to be(:dsn)
expect(setting.value).to eql('jdbc:sqlite')
end

it 'supports but deprecates giving a constructor as a block' do
expect(Dry::Core::Deprecations).to receive(:announce)
Copy link
Member

Choose a reason for hiding this comment

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

You can just set a logger that would write to log/deprecations.log and this way you could verify if the messages look good. It's an approach we've been using so far.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please, @solnic, take a look at 4e24d68. Do you mean like that? I used StringIO instead of an actual file to not have to mess with the filesystem. If that's what you meant, I'll squash both commits together.


setting = dsl.setting(:dsn, 'sqlite') { |value| "jdbc:#{value}" }

expect(setting.name).to be(:dsn)
Expand Down