search

jackdempsey / sequel_polymorphic

MIT License
37 stars 13 forks source link

Handle class option to one-to-many association #16

Closed shioyama closed 7 years ago

shioyama commented 7 years ago

This is quite a simple change and allows more flexible association/class name relationships.

jackdempsey commented 7 years ago

@shioyama Seems we should pass klass along if someone passes in a value. Update that and i'm fine with merging it.

shioyama commented 7 years ago

@jackdempsey I had that at first, but noticed no difference and this was simpler so just passed it as nil by default. But I'll make the change if you think that's better.

jackdempsey commented 7 years ago

Yeah, even if it works now, something may change and break in the future, etc. This should make it more bulletproof. Thanks!

shioyama commented 7 years ago

Ok updated, have a look :smile:

jackdempsey commented 7 years ago

Thanks!

kuraga commented 7 years ago

@shioyama @jackdempsey what about handling all options but not :class only?

association_options = { ... }.merge(options)

(in all methods)

shioyama commented 7 years ago

@kuraga Good point, I was only thinking about my immediate need. Would there be any issue with passing through options, i.e. could they affect polymorphic associations in a way that would break them? I guess not but I'm new to Sequel.

I can make another PR with the merge(options) approach.

kuraga commented 7 years ago

@shioyama we couldn't, user (programmer) could. But that's user's decision (pass some options).