Add where parameter to union_relations#554
Conversation
dbeatty10
left a comment
There was a problem hiding this comment.
Thanks for submitting this and for including both integration tests plus documentation 🏆
Thank you also for alerting about the branch names within the merge request template being out-of-date; those are updated now.
Review
Only feedback:
- I'd like to move the
whereparameter to be at the end of the argument list so we can guarantee no accidental breakage for any consumers.
Is there any reason why it would be beneficial or necessary to be the first keyword argument?
Question:
- It looks like the same filter specified by the
whereparameter will be applied to every relation; I'm assuming you haven't needed unique filters per relation as often?
Aside
At first, I was wondering if this would conflict or overlap with the where config, but it looks like it is only for test resources.
b2734cd to
aa96b18
Compare
|
@dbeatty10 Thanks for the feedback! I've made the changes and tests are passing. I rebased locally onto |
Sorry for the dramatic delay in conversation @LewisDavies Could I bother you for two more changes and then we can merge this in?
If you don't have the time, just let me know and I can do those two steps too. Thank you so much for your detailed solution on this! |
aa96b18 to
23fd24d
Compare
2f3b13d to
5bde6bc
Compare
|
@dbeatty10 No worries, the changes are all done. Since I rebased off main I updated the target branch too. With all my testing and rebasing the commits were an absolute mess, so I cleaned them up and force pushed again. Should be good to go now! |
dbeatty10
left a comment
There was a problem hiding this comment.
Looks awesome, @LewisDavies ! This rocks 🎸
Resolves #464, resolves #382
This is a:
maindev/branchdev/branchDescription & motivation
I often want to add basic filters when using
union_relations, such as excluding obsolete records or low-quality data. Currently, I have to create two models to do so: one to union the data then another to filter it. This PR adds awhereparameter tounion_relationsso models are filtered before being unioned.More context & discussion can be found in #464 and #382.
Checklist