Closed tnightengale closed 1 year ago
@bcodell I am going to break out the caller()
aggregation implementation into a separate PR.
@bcodell I think all your comments were on outdated commits; perhaps the page you started the review from was not refreshed?
Most of the comments were actually handled in the way you suggested in the latest commits. To this point:
Yeah this feels like the right approach. Does this argument always need to be a macro because of the way call blocks end up getting used? Or could someone pass a plain text string like 'sum' or 'list_agg'?
Yes, the always need to use the {{ caller() }}
builtin, strings will not work.
Also:
I like this better than the string-based approach, but if there's a possible future where more aggregation functions will be supported, then I think a better interface would be to create a registry of sorts to access all aggregations - similar to the columns() interface - so something like dbt_activity_schema.aggregations().min in this example. It's more verbose, but it feels more organized than having the different aggregation methods floating around at the top level of the package.
Yes, I agree we could namespace these more nicely 👍
Ah whoops - I added my approving PR through GitHub mobile and I probably misused the UI 🤷♂️
Nice work!
https://github.com/tnightengale/dbt-activity-schema/issues/9