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/generate multiple models yaml #85

Merged
merged 12 commits into from
Nov 23, 2022
Merged

Feature/generate multiple models yaml #85

merged 12 commits into from
Nov 23, 2022

Conversation

angelica-lastra
Copy link
Contributor

@angelica-lastra angelica-lastra commented Oct 24, 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

closes #68
Added code that allows for generate_model_yaml to accept multiple model arguments as a list, or one model as a string.
Updated README to reflect this feature.

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

Other comments

  • Should I add an entry to the CHANELOG.md for this?
  • Also, I am creating a pull request to merge into dev/ — should this be merged into main/ instead?

@bennieregenold7
Copy link

@angelica-lastra I think there may be something off between this and the branch you're merging into. There are 32 changed files, but if I remember correctly, you only changed one. Should this merge into dev/0.4.0 or main?

@angelica-lastra
Copy link
Contributor Author

@bennieregenold7 I think you are right, I got confused because the checklist on the PR said "[x] new functionality — please ensure the base branch is the latest dev/ branch", so I assumed I had to merge there, but main made more sense to me.

@angelica-lastra angelica-lastra changed the base branch from dev/0.4.0 to main October 24, 2022 20:02
@angelica-lastra
Copy link
Contributor Author

@bennieregenold7 I changed to merge to main and that seemed to address that concern, however, if anyone else has any guidance on this, please let me know!

bennieregenold7
bennieregenold7 reviewed Oct 24, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
graciegoheen
graciegoheen reviewed Oct 26, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
graciegoheen
graciegoheen approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@graciegoheen graciegoheen left a comment

Choose a reason for hiding this comment

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

Awesome work here! Especially love the way you dealt with appending the single model name to an empty list if it's inputed as a string. LGTM!

@angelica-lastra @graciegoheen
Update README.md
1ebff62 
Co-authored-by: Grace Goheen <53586774+graciegoheen@users.noreply.github.com>
@angelica-lastra
Copy link
Contributor Author

Awesome work here! Especially love the way you dealt with appending the single model name to an empty list if it's inputed as a string. LGTM!

@graciegoheen Thank you so much! I am so excited I have not stopped thinking about this! that particular code you are referring to is all thanks to @bennieregenold7 for catching it and walking me through how to write it!

joellabes
joellabes reviewed Oct 26, 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'm happy for this to go out as-is, but have a bit of feedback which you can take or leave! lmk what you think.

macros/generate_model_yaml.sql Outdated Show resolved Hide resolved
macros/generate_model_yaml.sql Outdated Show resolved Hide resolved
graciegoheen
graciegoheen reviewed Nov 3, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
joellabes
joellabes requested changes Nov 21, 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.

A wording tweak and a potential bug, then this looks good to go!

macros/generate_model_yaml.sql Outdated Show resolved Hide resolved
macros/generate_model_yaml.sql Show resolved Hide resolved
@joellabes joellabes merged commit 0a85cb2 into main Nov 23, 2022
@joellabes joellabes deleted the feature/generate_multiple_models_yaml branch November 23, 2022 22:01
@joellabes
Copy link
Contributor

hurrah! congrats @angelica-lastra 🎉

jeremyholtzman pushed a commit that referenced this pull request Apr 10, 2023
@joellabes
Merge pull request #85 from dbt-labs/feature/generate_multiple_models…
ddb3aa3 
…_yaml

Feature/generate multiple models yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graciegoheen graciegoheen graciegoheen approved these changes

@bennieregenold7 bennieregenold7 bennieregenold7 approved these changes

@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.

Support multiple arguments for generate_model_yaml
4 participants
@angelica-lastra @bennieregenold7 @joellabes @graciegoheen