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

Add struct composition #139

Merged
merged 1 commit into from Jan 19, 2020
Merged

Conversation

@waiting-for-dev
Copy link
Member

waiting-for-dev commented Jan 5, 2020

This feature would be a step towards using composition throughout dry-rb ecosystem, as we mentioned in dry-rb/dry-validation#595 comments

@flash-gordon

This comment has been minimized.

Copy link
Member

flash-gordon commented Jan 6, 2020

Why not reuse Dry::Struct.attributes() for that? I don't like compose since it implies you compose one with another and get something as a result. .attributes could check its argument and recognize a struct is passed. Or maybe add .attributes_from as a compromise

@waiting-for-dev

This comment has been minimized.

Copy link
Member Author

waiting-for-dev commented Jan 6, 2020

In fact I see it just like that, you compose one struct with another struct and you get a third one. In a way it's also a composition in contrast of inheritance: instead of descending from a parent struct you use another one as argument. But, actually, I also like seeing composition in a very functional way, and here we don't have this facet, as neither pieces nor result are something callable. What do you think about .merge instead of .attributes_from?

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jan 6, 2020

Please rebase this on top of master, otherwise checks won't be run

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev:compose branch from 0a39c15 to 78128e0 Jan 6, 2020
@waiting-for-dev

This comment has been minimized.

Copy link
Member Author

waiting-for-dev commented Jan 6, 2020

done @solnic

@flash-gordon

This comment has been minimized.

Copy link
Member

flash-gordon commented Jan 17, 2020

@waiting-for-dev merge is used for hashes and works differently. What about embed?

@timriley

This comment has been minimized.

Copy link
Member

timriley commented Jan 18, 2020

@flash-gordon

This comment has been minimized.

Copy link
Member

flash-gordon commented Jan 19, 2020

I'm going to pull this in under attributes_from. We can continue the discussion because the next version will be quite big and I will test thoroughly before cutting a release

@flash-gordon flash-gordon changed the base branch from master to merging-structs Jan 19, 2020
@flash-gordon flash-gordon merged commit c11be80 into dry-rb:merging-structs Jan 19, 2020
6 checks passed
6 checks passed
tests (2.7)
Details
tests (2.6)
Details
tests (2.5)
Details
tests (2.4)
Details
tests (jruby)
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.