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

Remove some deprecated macros #268

Merged
merged 4 commits into from
Aug 24, 2020
Merged

Remove some deprecated macros #268

merged 4 commits into from
Aug 24, 2020

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Aug 20, 2020

This is a:

  • bug fix PR with no breaking changes (please change the base branch to main)
  • new functionality
  • a breaking change

To do before merge

Description & motivation

I removed union_tables and get_tables_by_prefix, which we deprecated >6 months ago as part of v0.2.4 in Dec 2019

I also removed get_tables_by_pattern — this was introduced in v0.5.0 and is functionally equivalent to get_tables_by_pattern_sql. Given few people would be using this macro (as it's a helper macro), we are going to go straight to removing it rather than deprecating it first.

We could also consider deprecating:

  • The old arguments for surrogate_key macro: deprecated in v0.3.0 / April 2020
  • identifier: deprecation warning added in v0.4.0 / April 2020 (deprecated before then but not marked as such)

Personally, I think we should wait until the next version of utils to deprecate those

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • 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 the changelog

@clrcrl clrcrl changed the title Remove some deprevcated macros Remove some deprecated macros Aug 20, 2020
@clrcrl clrcrl requested a review from jtcohen6 August 20, 2020 20:13
Base automatically changed from feature/0-18-adapter-dispatch to dev/0.6.0 August 21, 2020 16:05
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

+0 −33

Nice! I agree, let's hold off on surrogate_key + identifier

@clrcrl clrcrl mentioned this pull request Aug 21, 2020
11 tasks
@clrcrl clrcrl force-pushed the refactor/remove-old-macros branch from 82cf0cb to 7aa8038 Compare August 22, 2020 21:04
@@ -10,7 +10,6 @@
{% endif -%}

{%- set column_override = column_override if column_override is not none else {} -%}
{%- set source_column_name = source_column_name if source_column_name is not none else '_dbt_source_relation' -%}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtcohen6 — can you just sense check this for me (related to line #1). I looked at it a number of times and can't figure out why it's like this.

I guess similarly, why do we have column_override=none instead of column_override={}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is because of the python convention to set default arguments to none, always, out of fear of mutable default arguments.

It's less of an issue with strings and dicts than with arrays, so I think it's probably fine to make this change.

@clrcrl clrcrl merged commit 7adb903 into dev/0.6.0 Aug 24, 2020
@clrcrl clrcrl deleted the refactor/remove-old-macros branch August 24, 2020 16:58
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