-
Notifications
You must be signed in to change notification settings - Fork 64
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
Polymorphic helpers #165
Polymorphic helpers #165
Conversation
70aea6d
to
b2211bc
Compare
Still needs a bit of work to support custom foreign keys for |
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 looks good from what I can tell. The only weird thing that I'm still sort of unsure of is where you call post || video
. If post
is nil, does it run a query first to see if that exists before checking video?
The “photo || video” calls the belongs to method just like normal. So you preload your polymorphic association which will make queried to get those associations. Which may or may not be nil. If photo is nil then it’ll try video and so on. So yeah it makes queries using the belongs to association. That’s why they need to be nilable. Since each association can be nil if it is polymorphic. Does that make sense? |
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.
Great docs!
it "type should not include nil" do | ||
task = PolymorphicTask::SaveOperation.create!(title: "Use Lucky") | ||
event = PolymorphicEvent::SaveOperation.create!(task_id: task.id) | ||
typeof(event.eventable).should eq(PolymorphicTask | PolymorphicTaskList) |
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.
You could also use be_a
(I think)
event.eventable.should be_a(PolymorphicTask | PolymorphicTaskList)
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 will give it a try!
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.
Ok so be_a
is for a single type as it operates on the runtime type and not compile time type. An example probably works best to describe this:
def mixed_array
[1, "foo"]
end
typeof(mixed_array) # Will be Array(String | Int32). typeof works at compile time. Does not actually call the method.
mixed_array.first # Will be String | Int32 at compile time, but at runtime it is one of the two, not *both*
mixed_array.first.should be_a(Int32) # We can only pass one type
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.
Interesting. Thanks for trying!
filled_attribute = attribute("filled") | ||
filled_attribute_2 = attribute("filled") | ||
blank_attribute = attribute("") | ||
Avram::Validations.validate_at_most_one_filled(filled_attribute, filled_attribute_2, blank_attribute) |
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.
This is cooool
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.
Glad you like it!
src/avram/polymorphic.cr
Outdated
# | ||
# ``` | ||
# Comment::BaseQuery.new.each do |comment| | ||
# comment.commentable! # Returns the photo or video |
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 we'd want them to preload explicitly more often than not, yeah? If so, I'd change this example to use preload_commentable
and the non-bang method
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.
Yeah that’s a good point. I was going for concise but that’s probably a better idea.
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.
Thank you for the reviews!
it "type should not include nil" do | ||
task = PolymorphicTask::SaveOperation.create!(title: "Use Lucky") | ||
event = PolymorphicEvent::SaveOperation.create!(task_id: task.id) | ||
typeof(event.eventable).should eq(PolymorphicTask | PolymorphicTaskList) |
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 will give it a try!
filled_attribute = attribute("filled") | ||
filled_attribute_2 = attribute("filled") | ||
blank_attribute = attribute("") | ||
Avram::Validations.validate_at_most_one_filled(filled_attribute, filled_attribute_2, blank_attribute) |
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.
Glad you like it!
src/avram/polymorphic.cr
Outdated
# This will generate methods for validiating, preloading, and lazy loading | ||
# polymorphic associations. | ||
# | ||
# Let's start with this example: |
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.
Good idea
src/avram/polymorphic.cr
Outdated
# | ||
# ``` | ||
# Comment::BaseQuery.new.each do |comment| | ||
# comment.commentable! # Returns the photo or video |
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.
Yeah that’s a good point. I was going for concise but that’s probably a better idea.
24563d4
to
e30000d
Compare
e30000d
to
a785c1b
Compare
Closes #145