Add AfterEagerFind()
#802
Add AfterEagerFind()
#802
Conversation
|
Not directly related to the topic of this PR, but I would like to postpone all new feature requests, especially adding new APIs until we clean up the current issues and solidify the design structure. There were, still are, a bunch of open issues and recently I worked on them to fix and/or categorize them. (see #770 and #773) During that time, I found that especially the code for the association and eager feature has no clear design direction and that could be one reason for the issue. I made some milestones for the clean-up, the sequence of them are:
Can we work on the backlog first along with the structural/design hardening before adding new features? I guess we could add new features related to the existing issues at some point in the plan/milestone when we can make sure of the solid fundamental design. (I think this could be a good approach since many of the issues happened or couldn't be closed early because we have a kind of... loose design concept. Doing this also will help us to make a better decision of direction on any particular issue) What do you think? Could you please share your idea? CC: @gobuffalo/core-managers |
|
Hey, totally understand that. We have struggled quite a bit with the relationship code ourselves. Too much reflection, and side effects. However, AfterFind is pretty much useless for any models that have associations. The related issues are open for over 3 years and it’s a very simple yet important change. Therefore, I strongly recommend to merge it. The changed code is minimal, and the pattern already exists. Alternatively we can also break BC and move AfterFind to have the associations loaded. Not fixing the problem for another year and seeing whether the work around the refactoring manifests will not work for us at least. |
|
Thanks for your opinion! I will take a look at the PR and your explanation as soon as possible! |
|
Appreciated :) |
There was a problem hiding this comment.
Hi @aeneasr, sorry for the delayed review. Overall, it looks good to me!
I have two tiny questions and added comments. I think the PR is already OK to be merged, but I would like to hear your thought about them. (If you agree but have no time to fix them, I will merge it as-is, and will fix them in a separate PR. Just let me know)
| if err := x.AfterEagerFind(c); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
It could be tiny. Since those two blocks are exclusive to each other, I would prefer to rewrite the block for readability as below. What do you think? (same for blocks in line 49)
if eager {
if x, ok := m.Value.(AfterFindable); ok {
if err := x.AfterFind(c); err != nil {
return err
}
}
} else {
if x, ok := m.Value.(AfterEagerFindable); ok {
if err := x.AfterEagerFind(c); err != nil {
return err
}
}
}There was a problem hiding this comment.
Basically just a habit of keeping nesting flat - would be fine either way :)
| }) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if q.eager { | ||
| err = q.eagerAssociations(model) | ||
| err := q.eagerAssociations(model) |
There was a problem hiding this comment.
Just curious if there is a specific reason for adding ':' here. If there is a specific reason, an inline comment could be useful.
There was a problem hiding this comment.
It's mostly a coding habit on my end, also fine either way
sio4
added
f: associations
|
Thank you for the review and merge @sio4 :) |
See #557
Closes #476