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

Deprecate constructor as a block #110

Closed
wants to merge 3 commits into from
Closed

Deprecate constructor as a block #110

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c81ea71
Deprecate constructor as a block
waiting-for-dev May 11, 2021
4e24d68
Test deprecation through a logger
waiting-for-dev May 14, 2021
73945a7
Deprecate removing visit_contructor method
waiting-for-dev May 15, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 11 additions & 0 deletions spec/unit/dry/configurable/dsl_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,21 @@
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
logger = StringIO.new
Dry::Core::Deprecations.logger(logger)
Copy link
Member

Choose a reason for hiding this comment

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

@waiting-for-dev you can move this to spec_helper and use Logger.new("log/deprecations.log") there

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless I'm missing something, this will make the logger persistent across tests, so we'll be sharing a state between them. I think it's not ideal unless we can't avoid it. What do you think creating about creating a helper method so that we can reuse it easily? For instance:

with_logger do |log|
  # expectation
  expect(log).to include('....')
end

Copy link
Member

Choose a reason for hiding this comment

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

@waiting-for-dev OK let's leave it here since it's the only place where we need it for now. Eventually we should have something reusable though, that can be provided by dry-core.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, it's going to be very soon, as I'm going to use it again in the PR that will deprecate providing the default as positional argument 😄


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

expect(setting.name).to be(:dsn)
expect(setting.value).to eql('jdbc:sqlite')
expect(logger.string).to include('constructor as a block is deprecated')
end

it 'compiles a nested list of settings' do
Expand Down