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

flip around generate_alias_name args, add node to generate_schema_name args #1483

Merged
merged 5 commits into from Jun 4, 2019

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented May 28, 2019

Add a node argument to the generate_schema_name macro, and makes the generate_alias_name macro compatible with that change.

Fixes #1483
Fixes #1463

It would be good to slip this in before people go out and build their own generate_alias_name macros!

@drewbanin drewbanin added this to the Wilt Chamberlain milestone May 29, 2019
@beckjake beckjake force-pushed the fix/swap-alias-generator-args branch from 8598a50 to 68782b7 Compare May 29, 2019 21:07
@beckjake beckjake changed the title flip around generate_alias_name args flip around generate_alias_name args, add node to generate_schema_name args May 29, 2019
@beckjake beckjake force-pushed the fix/swap-alias-generator-args branch from 00559fd to 1278662 Compare May 30, 2019 16:03
Copy link
Contributor Author

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

One question and a warning update, but this otherwise works great and LGTM!

)
if too_many_args not in str(exc):
raise
msg = ('The generate_schema_name macro does not accept a second '
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you change this to

As of dbt v0.14.0, the `generate_schema_name` macro accepts a second "node" argument.
The one-argument form of `generate_schema_name` is deprecated, and will become
unsupported in a future release.

For more information, see: https://docs.getdbt.com/v0.14/docs/upgrading-to-014

What's the benefit of rolling this as a warning (with repeat=False) over using a new Deprecation class?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I had completely forgotten about the existence of deprecations somehow. I guess I haven't had to think about them in a while...

Copy link
Contributor Author

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Super nitpicky feedback on the deprecation warning. It should read like:

Deprecation Warning: As of dbt v0.14.0, the `generate_schema_name` macro
  accepts a second "node" argument. The one-argument form of `generate_schema_name`
  is deprecated, and will become unsupported in a future release.

Merge once that's updated!

core/dbt/deprecations.py Outdated Show resolved Hide resolved
core/dbt/deprecations.py Outdated Show resolved Hide resolved
core/dbt/deprecations.py Outdated Show resolved Hide resolved
drewbanin and others added 5 commits June 3, 2019 16:32
Fix many tests
Support single-arg generate_schema_name macros
Add repeat flag to warn_or_error to suppress duplicate warnings
Add a warning if a user's macro does not take a second argument
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.

Pass parsed_node into generate_schema_name.
2 participants