-
Notifications
You must be signed in to change notification settings - Fork 149
[codex] Improve Jinja macro guardrails #405
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, why? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,8 @@ | ||
| {% macro snowflake__refresh_external_table(source_node) %} | ||
|
|
||
| {% set external = source_node.external %} | ||
| {% set snowpipe = source_node.external.get('snowpipe', none) %} | ||
|
|
||
| {% set auto_refresh = external.get('auto_refresh', false) %} | ||
| {% set partitions = external.get('partitions', none) %} | ||
| {% set delta_format = (external.table_format | lower == "delta") %} | ||
|
Comment on lines
-4
to
-8
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these do exist in the snowpipe tables which also get refreshed with refresh_external_table btw |
||
|
|
||
| {% set manual_refresh = not auto_refresh %} | ||
|
|
||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does this file do?! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| from pathlib import Path | ||
| from unittest import TestCase | ||
|
|
||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||
|
|
||
|
|
||
| def read_macro(path: str) -> str: | ||
| return (REPO_ROOT / path).read_text() | ||
|
|
||
|
|
||
| class JinjaMacroContractsTest(TestCase): | ||
| def test_bigquery_connection_name_does_not_mutate_options(self): | ||
| macro = read_macro("macros/plugins/bigquery/create_external_table.sql") | ||
|
|
||
| self.assertNotIn(".pop(", macro) | ||
| self.assertIn("key not in excluded_options", macro) | ||
|
|
||
| def test_redshift_empty_partitions_checked_before_logging_range(self): | ||
| macro = read_macro("macros/plugins/redshift/helpers/add_partitions.sql") | ||
|
|
||
| empty_guard = macro.index("if partitions|length > 0") | ||
| first_partition_lookup = macro.index("partitions[0]") | ||
| self.assertLess(empty_guard, first_partition_lookup) | ||
|
|
||
| def test_fabric_nullity_uses_current_column_tests(self): | ||
| macro = read_macro("macros/plugins/fabric/create_external_table.sql") | ||
|
|
||
| self.assertNotIn("columns.tests", macro) | ||
| self.assertIn("column_tests", macro) | ||
|
|
||
| def test_snowflake_refresh_has_no_unused_branch_variables(self): | ||
| macro = read_macro("macros/plugins/snowflake/refresh_external_table.sql") | ||
|
|
||
| self.assertNotIn("set snowpipe", macro) | ||
| self.assertNotIn("set partitions", macro) | ||
| self.assertNotIn("set delta_format", macro) | ||
|
Comment on lines
+32
to
+37
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there even a pytest harness in this repo?? |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i like this but say more