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.

Sign up for GitHub

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

Jump to bottom

Feature/tables option for codegen #60

Merged
merged 14 commits into from
Mar 22, 2022
Merged

Feature/tables option for codegen #60

merged 14 commits into from
Mar 22, 2022

Conversation

matt-winkler
Copy link
Contributor

@matt-winkler matt-winkler commented Mar 8, 2022 •
edited
Loading

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is main
  • new functionality — please ensure the base branch is the latest dev/ branch
  • a breaking change — please ensure the base branch is the latest dev/ branch

Description & motivation

See here

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@matt-winkler matt-winkler marked this pull request as ready for review March 9, 2022 17:28
joellabes
joellabes requested changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

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

Looking good! A couple of compatibility and testing requests, but otherwise very on board with getting this merged in 🥳

macros/generate_source.sql Outdated Show resolved Hide resolved
integration_tests/tests/test_generate_source_table_pattern.sql Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@matt-winkler
Copy link
Contributor Author

matt-winkler commented Mar 14, 2022 •
edited
Loading

@joellabes Good feedback! I was wondering about the use of exclude here as well, so went ahead and added that one in + some love on integration tests.

I also moved the table_pattern and exclude parameters to the end of the macro args list in order to make a new minor version an option. Originally, was biasing towards matching the warehouse hierarchy (from lower to higher grain), but schema being ahead of database and required / non-default kind of defeats the purpose of that. Happy to move this around again if you prefer & update the Changelog accordingly.

joellabes
joellabes approved these changes Mar 22, 2022
View reviewed changes
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

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

I like where you've landed with this! Love the pair of test cases with both a specific inclusion and an exclusion 😍

Let's ship it 🚢

@joellabes joellabes merged commit 14a92ad into main Mar 22, 2022
@matt-winkler matt-winkler mentioned this pull request May 4, 2022
Closed
jeremyholtzman pushed a commit that referenced this pull request Apr 10, 2023
@joellabes
Merge pull request #60 from dbt-labs/feature/tables-option-for-codegen
c047fca 
Feature/tables option for codegen
@gwenwindflower gwenwindflower deleted the feature/tables-option-for-codegen branch February 28, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joellabes joellabes joellabes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matt-winkler @joellabes