-
-
Notifications
You must be signed in to change notification settings - Fork 62
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 nested structs by passing a block to Dry::Struct.attribute #58
Conversation
lib/dry/struct/class_interface.rb
Outdated
# ruby.details.type #=> 'OO' | ||
def attribute(name, type = nil, &block) | ||
if block | ||
type = build_nested_type(type || ::Dry::Struct, &block) |
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.
Not sure whether type should default to the superclass of the type defining the nested attribute?
NVM, that would kill type inheritance
@AMHOL This looks awesome. Pretty useful for complex structures |
@AMHOL I like it, I think we could even create a constant for dynamically created types so that they have nicer names in introspection. Since we already have a namespace (aka class/namespace) we're free to do so. class User < Dry::Struct
attribute :name, 'strict.string'
attribute :address do
attribute :city, 'strict.string'
attribute :zip, 'strict.string'
end
end
User::Address.new(city: 'Coruscant', zip: '1234567') WDYT? |
@flash-gordon its a cool idea but I don't think we should dynamically generate constants like that, its likely to cause issues if somebody decides to define the same constant manually, which I think is likely to happen with something like the example scenario. |
@AMHOL could you explain why somebody would do so? Because I have quite a bit of code that can be simplified using the proposed syntax but I can't see any example of defining a struct inline and then having a named struct with a different definition. |
Hmm, actually you're right, it would be kind of strange for someone to do that, but if you have multiple cases for the same nested attribute then wouldn't extracting it and defining it manually make sense? I just don't like the idea of a library DSL defining constants based on method calls when its not obvious that it will do. edited |
@flash-gordon I thought about this some more last night and spoke with @timriley this morning and I can see where you're coming from with this. Do we think this should be implemented as default behaviour or opt-in, i.e. # Defines User::Address
class User < Dry::Struct
attribute :address, as: 'Address' do
# nested attributes
end
end Or perhaps as default behaviour with an opt-out, i.e. # Does not define User::Address
class User < Dry::Struct
attribute :address, anonymous: true do
# nested attributes
end
end Personally, I prefer the opt-in method, as we don't have to infer a constant name and its explicit. CC: @solnic @GustavoCaso |
My 2c: I reckon the dynamically created types, as @flash-gordon described, would be really nice. I think it would be fine as default, too. I'm not sure we need to build an opt-out. Maybe just raise an exception if a conflicting constant is found? |
# @raise [RepeatedAttributeError] when trying to define attribute with the | ||
# same name as previously defined one | ||
# | ||
# @example | ||
# class Language < Dry::Struct | ||
# attribute :name, Types::String | ||
# attribute :details, Dry::Struct do |
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.
@AMHOL is attribute :details, Dry::Struct do ... end
expected usage still? I thought we wouldn't need to pass a type like Dry::Struct
when supplying nested attributes? It seems redundant.
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.
@timriley that's optional, but allows you to define the parent type of the nested attribute, useful for extending existing types or defining a base type with custom configuration.
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.
@AMHOL OK, this makes sense. That's quite handy! So I suppose if the user supplied a type here, none of the default auto-class-defining behaviour would kick in?
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.
It would still kick in as normal, the only thing that passing this argument changes is the superclass of the nested type that is defined, so for example, if I do the following:
class Address < Dry::Struct
attribute :city, Types::Coercible::String
attribute :zip, Types::Coercible::String
end
class User < Dry::Struct
attribute :name, Types::Coercible::String
attribute :age, Types::Coercible::Int
attribute :address, Address do
attribute :line_1, Types::Coercible::String
attribute :line_2, Types::Coercible::String
end
end
It will still define User::Address
, and User::Address
will descend from Address
and have the attributes line_1
, line_2
, city
, zip
, as if it were defined like:
class User < Dry::Struct
class Address < ::Address
attribute :line_1, Types::Coercible::String
attribute :line_2, Types::Coercible::String
end
end
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.
Wonderful!
Good to merge then? |
5be118e
to
6022e3c
Compare
Define constants for nested attributes
6022e3c
to
6f2bd1a
Compare
LGTM |
As discussed in #2
Examples
You can also pass a superclass type, this will be used as the superclass of the nested type that is built, i.e.
This is useful for both scenarios where you have a custom type that you would like to extend for a one-off use-case as well as defining a base type to set the
constructor_type
CC: @solnic @timriley @flash-gordon @GustavoCaso