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 `Dry::Types::Hash::Schema.merge` #381

Merged
merged 1 commit into from Jan 6, 2020
Merged

Add `Dry::Types::Hash::Schema.merge` #381

merged 1 commit into from Jan 6, 2020

Conversation

@waiting-for-dev
Copy link
Member

waiting-for-dev commented Jan 5, 2020

It's also aliased as .compose.

I submit this PR in relation with dry-rb/dry-struct#139

It moves composition to the fundamental piece of a Dry::Struct, a Dry::Types::Hash::Schema.

If this PR is accepted I'll modify the other one accordingly so it makes use of the new method.

Key transformation and type transformation work in a different way due to how they are implemented, but I haven't studied why it is this way. Any feedback is welcomed :)

@waiting-for-dev waiting-for-dev force-pushed the merge_schemas branch from 999016c to 8535da0 Jan 5, 2020
@waiting-for-dev waiting-for-dev changed the title Add `Dry::Struct.merge` Add `Dry::Types::Schema.merge` Jan 5, 2020
@waiting-for-dev waiting-for-dev force-pushed the merge_schemas branch from 8535da0 to 7d1965c Jan 5, 2020
@waiting-for-dev waiting-for-dev changed the title Add `Dry::Types::Schema.merge` Add `Dry::Types::Hash::Schema.merge` Jan 5, 2020
@waiting-for-dev

This comment has been minimized.

Copy link
Member Author

waiting-for-dev commented Jan 5, 2020

Hey @solnic, a downside of this automation of Codacy you added is that its notifications come with your name (and so, consequently, the notification emails)... This way it is easy to miss a more personalized and important message written by you (as probably people will let automatic code review messages for later)

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jan 5, 2020

Hey @solnic, a downside of this automation of Codacy you added is that its notifications come with your name (and so, consequently, the notification emails)... This way it is easy to miss a more personalized and important message written by you (as probably people will let automatic code review messages for later)

that's not good. I'll see if this can be changed, if not - we'll go back to houndci.

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jan 5, 2020

@waiting-for-dev could you rebase this on top of master please?

@dry-rb dry-rb deleted a comment from hound bot Jan 5, 2020
@dry-rb dry-rb deleted a comment from hound bot Jan 5, 2020
@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jan 5, 2020

@waiting-for-dev I disabled codeclimate and houndci and I also configured dry-bot to be the author of codacy comments. We'll see how that works once you push something new here 😄

@waiting-for-dev waiting-for-dev force-pushed the merge_schemas branch 2 times, most recently from 5335108 to 59abfe9 Jan 6, 2020
@waiting-for-dev

This comment has been minimized.

Copy link
Member Author

waiting-for-dev commented Jan 6, 2020

@solnic I rebased to master and fixed two issues I introduced by mistake.

Yep! Codacy is now much helpful 😃

Copy link
Member

flash-gordon left a comment

It's a nice addition @waiting-for-dev 👍 I left a couple of comments but overall it's good

def merge(schema)
with(keys: keys + schema.keys)
end
alias_method :compose, :merge

This comment has been minimized.

Copy link
@flash-gordon

flash-gordon Jan 6, 2020

Member

Let's not add it for now. Conceptually, it's okay, but from my experience, the fewer methods dry-types has, the better. That said, we can add it later if we see it'll be useful in other gems.

This comment has been minimized.

Copy link
@waiting-for-dev

waiting-for-dev Jan 6, 2020

Author Member

That's ok. I just found sweet having dry-struct's #compose (the other PR I referenced) calling underneath schema's #compose, but it's not such a big deal 😄

#
# @api public
def merge(schema)
with(keys: keys + schema.keys)

This comment has been minimized.

Copy link
@flash-gordon

flash-gordon Jan 6, 2020

Member

repeated names should be filtered here. Otherwise, you can end up with keys having duplicates, this is not good.

@flash-gordon

This comment has been minimized.

Copy link
Member

flash-gordon commented Jan 6, 2020

@waiting-for-dev oh, hold on a sec. We have Schema#schema already, we should just call schema(other.keys) in merge. WDYT?

@waiting-for-dev waiting-for-dev force-pushed the merge_schemas branch from 59abfe9 to b744c5d Jan 6, 2020
@waiting-for-dev

This comment has been minimized.

Copy link
Member Author

waiting-for-dev commented Jan 6, 2020

Thanks for your review @flash-gordon , I forced a new push

@flash-gordon flash-gordon merged commit 1a2f774 into master Jan 6, 2020
11 checks passed
11 checks passed
tests-mri (2.6.x, true)
Details
tests-mri (2.6.x, false)
Details
tests-mri (2.5.x, true)
Details
tests-mri (2.5.x, false)
Details
tests-mri (2.4.x, true)
Details
tests-mri (2.4.x, false)
Details
tests-others (jruby:9.2.9, true)
Details
tests-others (jruby:9.2.9, false)
Details
tests-others (ruby:rc, true)
Details
tests-others (ruby:rc, false)
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
@flash-gordon flash-gordon deleted the merge_schemas branch Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.