-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add attribute readers for unknown attributes #89
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# frozen_string_literal: true | ||
|
||
describe "required param" do | ||
before do | ||
class Test::Foo | ||
extend Dry::Initializer | ||
|
||
param :foo | ||
param :bar, optional: true | ||
end | ||
end | ||
|
||
it "raise ArgumentError" do | ||
expect { Test::Foo.new }.to raise_exception(ArgumentError) | ||
end | ||
end | ||
|
||
describe "required option" do | ||
before do | ||
class Test::Foo | ||
extend Dry::Initializer | ||
|
||
option :foo | ||
option :bar, optional: true | ||
end | ||
end | ||
|
||
it "raise ArgumentError" do | ||
expect { Test::Foo.new }.to raise_exception(ArgumentError) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
# frozen_string_literal: true | ||
|
||
describe "attribute with several assignments" do | ||
before do | ||
class Test::Foo | ||
extend Dry::Initializer | ||
|
||
option :bar, proc(&:to_s), optional: true | ||
option :"some foo", as: :bar, optional: true | ||
option :bar, proc(&:to_s), optional: true | ||
option :some_foo, as: :bar, optional: true | ||
end | ||
end | ||
|
||
|
@@ -13,7 +15,7 @@ class Test::Foo | |
|
||
it "is left undefined" do | ||
expect(subject.bar).to be_nil | ||
expect(subject.instance_variable_get :@bar) | ||
expect(subject.instance_variable_get(:@bar)) | ||
.to eq Dry::Initializer::UNDEFINED | ||
end | ||
end | ||
|
@@ -27,7 +29,7 @@ class Test::Foo | |
end | ||
|
||
context "when renamed" do | ||
subject { Test::Foo.new "some foo": :BAZ } | ||
subject { Test::Foo.new some_foo: :BAZ } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And the same problem is here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why I originally made this change. Perhaps some intermediate state. I've reverted its and specs still pass. Can you approve the actions here so we can get spec output? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, I meant a different change. The original version got a space inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK I missed that. The challenge with supporting that is we can't have options like that explicitly defined in the signature. We'd have to bury that logic in unpacking the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The particular challenge being Introspecting the dry initializer doesn't accurately return the constructor arguments |
||
|
||
it "renames the attribute" do | ||
expect(subject.bar).to eq :BAZ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String keyword argument names aren't allowed in 2.6. This is perhaps a very small breaking change but frankly its unlikely anyone on 2.6 was taking advantage of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here is a potential for the backward incompatibility. This test is about not string, but symbol keys. What is essential is that the keys don't need to be a valid ruby methods.
For example, we should can handle this assignment:
The goal of this feature to make it possible handling any keys in the income hash (maybe with a forced symbolization like below)