Skip to content
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

Bugfix/missing schema columns #48

Merged
merged 8 commits into from
Jan 17, 2025

Conversation

awoehrl
Copy link
Contributor

@awoehrl awoehrl commented Dec 20, 2023

@bcodell, I found a bug in my codegen macro.

I missed that the schema_columns macro doesn't contain the anonymous_customer_id column. Also I realized, I was using customer_id, when the activity schema specs use customer for the customer column.

Right now this means, that customer and anonymous_customer_id won't get renamed correctly if aliased.

My further question would be:

  • Would it make sense to have a global macro that delivers all columns, aliases and a flag if the field is used for an activity?

@bcodell
Copy link
Owner

bcodell commented Dec 27, 2023

Would it make sense to have a global macro that delivers all columns, aliases and a flag if the field is used for an activity?

Hey @awoehrl this is a great callout! As part of my work to provide support for anonymous_customer_id, I'm going to update the schema_columns macro to accept an optional argument for stream. If passed, then the macro will return a dictionary that contains the appropriate metadata for the customer and anonymous_customer_id columns. I'll follow up here once that's merged so you can put it to use in the code generation macro. Thanks!

@bcodell
Copy link
Owner

bcodell commented Dec 28, 2023

Hey @awoehrl the schema_columns now accepts an optional stream argument, which will prompt the macro to return appropriate metadata for all columns. Give it a go in this PR!

@awoehrl awoehrl marked this pull request as ready for review January 8, 2024 14:45
@bcodell
Copy link
Owner

bcodell commented Feb 1, 2024

Hey @awoehrl - thanks for the updates! And I appreciate you adding a model to use for testing 🙏 Looks like these changes broke CI though. I'm going to pull down to my local machine to see if this issue is related to CI running on a forked branch. If that's the case, then we should be good to merge! If not, given that this is a utility function, I'd be okay with deprecating the testing and just shipping the changes to the macro.

Copy link
Owner

@bcodell bcodell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better (very) late than never... confirming the failed CI is related to secrets access. Sorry for leaving this hanging @awoehrl and thanks for contributing!

@bcodell bcodell merged commit 9fe510e into bcodell:main Jan 17, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants