From 10d2a708f877722ed9a61edd8a78933764306972 Mon Sep 17 00:00:00 2001 From: Felipe Guilherme Sabino <982190+sabino@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:24:01 -0300 Subject: [PATCH 1/8] feat: allow column description to be overwritten --- dbtmetabase/manifest.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/dbtmetabase/manifest.py b/dbtmetabase/manifest.py index f4908491..42567736 100644 --- a/dbtmetabase/manifest.py +++ b/dbtmetabase/manifest.py @@ -26,6 +26,7 @@ _COMMON_META_FIELDS = [ "display_name", "visibility_type", + "description", ] # Must be covered by Column attributes _COLUMN_META_FIELDS = _COMMON_META_FIELDS + [ @@ -136,14 +137,17 @@ def _read_column( schema: str, relationship: Optional[Mapping], ) -> Column: - column = Column( - name=manifest_column.get("name", ""), - description=manifest_column.get("description"), - **self._scan_fields( - manifest_column.get("meta", {}), + + scanned_fields = self._scan_fields( + manifest_column.get("meta", {}), fields=_COLUMN_META_FIELDS, ns=_META_NS, - ), + ) + + column = Column( + name=manifest_column.get("name", ""), + description=scanned_fields.get("description", manifest_column.get("description")), + **{key: value for key, value in scanned_fields.items() if key != "description"}, ) self._set_column_relationship( From c3a2bc9ee7258e85b933a756e3b1116ea7889844 Mon Sep 17 00:00:00 2001 From: sabino <982190+sabino@users.noreply.github.com> Date: Tue, 26 Nov 2024 09:38:44 -0300 Subject: [PATCH 2/8] feat: Consider Model description as well --- dbtmetabase/manifest.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dbtmetabase/manifest.py b/dbtmetabase/manifest.py index 42567736..9ad4f2fd 100644 --- a/dbtmetabase/manifest.py +++ b/dbtmetabase/manifest.py @@ -111,6 +111,12 @@ def _read_model( for column in manifest_model.get("columns", {}).values() ] + scanned_fields = self._scan_fields( + manifest_model.get("meta", {}), + fields=_MODEL_META_FIELDS, + ns=_META_NS, + ) + return Model( database=database, schema=schema, @@ -119,16 +125,12 @@ def _read_model( alias=manifest_model.get( "alias", manifest_model.get("identifier", manifest_model["name"]) ), - description=manifest_model.get("description"), + description=scanned_fields.get("description", manifest_model.get("description")), columns=columns, unique_id=unique_id, source=source, tags=manifest_model.get("tags", []), - **self._scan_fields( - manifest_model.get("meta", {}), - fields=_MODEL_META_FIELDS, - ns=_META_NS, - ), + **{key: value for key, value in scanned_fields.items() if key != "description"}, ) def _read_column( From 8c2a86aacc7874b0de47b4fe3300c384a1ef615d Mon Sep 17 00:00:00 2001 From: sabino <982190+sabino@users.noreply.github.com> Date: Tue, 26 Nov 2024 09:39:12 -0300 Subject: [PATCH 3/8] chore: Update tests --- tests/fixtures/manifest-v12.json | 15 ++++++++++----- tests/test_manifest.py | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/fixtures/manifest-v12.json b/tests/fixtures/manifest-v12.json index c93bcbfa..a3dc3197 100644 --- a/tests/fixtures/manifest-v12.json +++ b/tests/fixtures/manifest-v12.json @@ -36,7 +36,9 @@ "schema": null, "database": null, "tags": [], - "meta": {}, + "meta": { + "metabase.description": "This table has basic information about payments" + }, "group": null, "materialized": "table", "incremental_strategy": null, @@ -62,7 +64,6 @@ "access": "protected" }, "tags": [], - "description": "This table has basic information about payments", "columns": { "payment_id": { "name": "payment_id", @@ -121,7 +122,9 @@ "tags": [] } }, - "meta": {}, + "meta": { + "metabase.description": "This table has basic information about payments" + }, "group": null, "docs": { "show": true, @@ -193,7 +196,8 @@ "database": null, "tags": [], "meta": { - "metabase.display_name": "clients" + "metabase.display_name": "clients", + "metabase.description": "Contains customer details and derived order facts" }, "group": null, "materialized": "table", @@ -290,7 +294,8 @@ } }, "meta": { - "metabase.display_name": "clients" + "metabase.display_name": "clients", + "metabase.description": "Contains customer details and derived order facts" }, "group": null, "docs": { diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 938390cc..de3e8987 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -60,7 +60,7 @@ def test_v12(): group=Group.nodes, name="customers", alias="customers", - description="This table has basic information about a customer, as well as some derived facts based on a customer's orders", + description="Contains customer details and derived order facts", display_name="clients", unique_id="model.sandbox.customers", columns=[ From bbeb8755f08513fdd1cce6045b68fe6c4a4ca5e7 Mon Sep 17 00:00:00 2001 From: sabino <982190+sabino@users.noreply.github.com> Date: Tue, 26 Nov 2024 10:04:28 -0300 Subject: [PATCH 4/8] chore: Column test --- tests/fixtures/manifest-v12.json | 4 +++- tests/test_manifest.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/fixtures/manifest-v12.json b/tests/fixtures/manifest-v12.json index a3dc3197..996709b8 100644 --- a/tests/fixtures/manifest-v12.json +++ b/tests/fixtures/manifest-v12.json @@ -87,7 +87,9 @@ "payment_method": { "name": "payment_method", "description": "", - "meta": {}, + "meta": { + "metabase.description": "The method used to complete a payment." + }, "data_type": null, "constraints": [], "quote": null, diff --git a/tests/test_manifest.py b/tests/test_manifest.py index de3e8987..816272a9 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -39,7 +39,7 @@ def test_v12(): ), Column( name="payment_method", - description="", + description="The method used to complete a payment.", ), Column( name="order_id", From 135142e3aca28e468036507c4dd44248d7639d4c Mon Sep 17 00:00:00 2001 From: sabino <982190+sabino@users.noreply.github.com> Date: Tue, 26 Nov 2024 19:12:37 -0300 Subject: [PATCH 5/8] fix: fmt --- dbtmetabase/manifest.py | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/dbtmetabase/manifest.py b/dbtmetabase/manifest.py index 9ad4f2fd..6c87d19f 100644 --- a/dbtmetabase/manifest.py +++ b/dbtmetabase/manifest.py @@ -112,11 +112,11 @@ def _read_model( ] scanned_fields = self._scan_fields( - manifest_model.get("meta", {}), - fields=_MODEL_META_FIELDS, - ns=_META_NS, + manifest_model.get("meta", {}), + fields=_MODEL_META_FIELDS, + ns=_META_NS, ) - + return Model( database=database, schema=schema, @@ -125,12 +125,18 @@ def _read_model( alias=manifest_model.get( "alias", manifest_model.get("identifier", manifest_model["name"]) ), - description=scanned_fields.get("description", manifest_model.get("description")), + description=scanned_fields.get( + "description", manifest_model.get("description") + ), columns=columns, unique_id=unique_id, source=source, tags=manifest_model.get("tags", []), - **{key: value for key, value in scanned_fields.items() if key != "description"}, + **{ + key: value + for key, value in scanned_fields.items() + if key != "description" + }, ) def _read_column( @@ -139,17 +145,22 @@ def _read_column( schema: str, relationship: Optional[Mapping], ) -> Column: - scanned_fields = self._scan_fields( manifest_column.get("meta", {}), - fields=_COLUMN_META_FIELDS, - ns=_META_NS, + fields=_COLUMN_META_FIELDS, + ns=_META_NS, ) - + column = Column( name=manifest_column.get("name", ""), - description=scanned_fields.get("description", manifest_column.get("description")), - **{key: value for key, value in scanned_fields.items() if key != "description"}, + description=scanned_fields.get( + "description", manifest_column.get("description") + ), + **{ + key: value + for key, value in scanned_fields.items() + if key != "description" + }, ) self._set_column_relationship( From bd849ddc27ea89c415ba85b328032ab59644482d Mon Sep 17 00:00:00 2001 From: sabino <982190+sabino@users.noreply.github.com> Date: Tue, 26 Nov 2024 21:42:46 -0300 Subject: [PATCH 6/8] fix: test v2 manifest instead of v12 --- tests/fixtures/manifest-v12.json | 19 ++++++------------- tests/fixtures/manifest-v2.json | 8 ++++++-- tests/test_manifest.py | 8 ++++---- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/tests/fixtures/manifest-v12.json b/tests/fixtures/manifest-v12.json index 996709b8..c93bcbfa 100644 --- a/tests/fixtures/manifest-v12.json +++ b/tests/fixtures/manifest-v12.json @@ -36,9 +36,7 @@ "schema": null, "database": null, "tags": [], - "meta": { - "metabase.description": "This table has basic information about payments" - }, + "meta": {}, "group": null, "materialized": "table", "incremental_strategy": null, @@ -64,6 +62,7 @@ "access": "protected" }, "tags": [], + "description": "This table has basic information about payments", "columns": { "payment_id": { "name": "payment_id", @@ -87,9 +86,7 @@ "payment_method": { "name": "payment_method", "description": "", - "meta": { - "metabase.description": "The method used to complete a payment." - }, + "meta": {}, "data_type": null, "constraints": [], "quote": null, @@ -124,9 +121,7 @@ "tags": [] } }, - "meta": { - "metabase.description": "This table has basic information about payments" - }, + "meta": {}, "group": null, "docs": { "show": true, @@ -198,8 +193,7 @@ "database": null, "tags": [], "meta": { - "metabase.display_name": "clients", - "metabase.description": "Contains customer details and derived order facts" + "metabase.display_name": "clients" }, "group": null, "materialized": "table", @@ -296,8 +290,7 @@ } }, "meta": { - "metabase.display_name": "clients", - "metabase.description": "Contains customer details and derived order facts" + "metabase.display_name": "clients" }, "group": null, "docs": { diff --git a/tests/fixtures/manifest-v2.json b/tests/fixtures/manifest-v2.json index ead8cc04..4bda07be 100644 --- a/tests/fixtures/manifest-v2.json +++ b/tests/fixtures/manifest-v2.json @@ -99,7 +99,9 @@ "status": { "name": "status", "description": "Orders can be one of the following statuses:\n\n| status | description |\n|----------------|------------------------------------------------------------------------------------------------------------------------|\n| placed | The order has been placed but has not yet left the warehouse |\n| shipped | The order has ben shipped to the customer and is currently in transit |\n| completed | The order has been received by the customer |\n| return_pending | The customer has indicated that they would like to return the order, but it has not yet been received at the warehouse |\n| returned | The order has been returned by the customer and received at the warehouse |", - "meta": {}, + "meta": { + "metabase.description": "Order status: placed, shipped, completed, return_pending, or returned." + }, "data_type": null, "quote": null, "tags": [] @@ -145,7 +147,9 @@ "tags": [] } }, - "meta": {}, + "meta": { + "metabase.description": "Basic and derived order information from payments" + }, "docs": { "show": true }, diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 816272a9..e8755799 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -39,7 +39,7 @@ def test_v12(): ), Column( name="payment_method", - description="The method used to complete a payment.", + description="", ), Column( name="order_id", @@ -60,7 +60,7 @@ def test_v12(): group=Group.nodes, name="customers", alias="customers", - description="Contains customer details and derived order facts", + description="This table has basic information about a customer, as well as some derived facts based on a customer's orders", display_name="clients", unique_id="model.sandbox.customers", columns=[ @@ -258,7 +258,7 @@ def test_v2(): group=Group.nodes, name="orders", alias="orders", - description="This table has basic information about orders, as well as some derived facts based on payments", + description="Basic and derived order information from payments", unique_id="model.jaffle_shop.orders", columns=[ Column( @@ -278,7 +278,7 @@ def test_v2(): ), Column( name="status", - description="Orders can be one of the following statuses:\n\n| status | description |\n|----------------|------------------------------------------------------------------------------------------------------------------------|\n| placed | The order has been placed but has not yet left the warehouse |\n| shipped | The order has ben shipped to the customer and is currently in transit |\n| completed | The order has been received by the customer |\n| return_pending | The customer has indicated that they would like to return the order, but it has not yet been received at the warehouse |\n| returned | The order has been returned by the customer and received at the warehouse |", + description="Order status: placed, shipped, completed, return_pending, or returned.", ), Column( name="amount", From 3e01490387468f9ec089468e9186783af527793f Mon Sep 17 00:00:00 2001 From: Mike Gouline <1960272+gouline@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:26:42 +1100 Subject: [PATCH 7/8] Apply suggestions from code review --- dbtmetabase/manifest.py | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/dbtmetabase/manifest.py b/dbtmetabase/manifest.py index 6c87d19f..1c8e7563 100644 --- a/dbtmetabase/manifest.py +++ b/dbtmetabase/manifest.py @@ -111,12 +111,12 @@ def _read_model( for column in manifest_model.get("columns", {}).values() ] - scanned_fields = self._scan_fields( + meta = self._scan_fields( manifest_model.get("meta", {}), fields=_MODEL_META_FIELDS, ns=_META_NS, ) - + description = meta.pop("description", manifest_model.get("description")) return Model( database=database, schema=schema, @@ -132,11 +132,7 @@ def _read_model( unique_id=unique_id, source=source, tags=manifest_model.get("tags", []), - **{ - key: value - for key, value in scanned_fields.items() - if key != "description" - }, + **meta, ) def _read_column( @@ -145,22 +141,16 @@ def _read_column( schema: str, relationship: Optional[Mapping], ) -> Column: - scanned_fields = self._scan_fields( + meta = self._scan_fields( manifest_column.get("meta", {}), fields=_COLUMN_META_FIELDS, ns=_META_NS, ) - + description = meta.get("description", manifest_column.get("description")) column = Column( name=manifest_column.get("name", ""), - description=scanned_fields.get( - "description", manifest_column.get("description") - ), - **{ - key: value - for key, value in scanned_fields.items() - if key != "description" - }, + description=description, + **meta, ) self._set_column_relationship( From 71a157db10892b36dc7966d57f6c4ec2946df3d4 Mon Sep 17 00:00:00 2001 From: Mike Gouline <1960272+gouline@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:27:12 +1100 Subject: [PATCH 8/8] Apply suggestions from code review --- dbtmetabase/manifest.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dbtmetabase/manifest.py b/dbtmetabase/manifest.py index 1c8e7563..d78e86b8 100644 --- a/dbtmetabase/manifest.py +++ b/dbtmetabase/manifest.py @@ -125,9 +125,7 @@ def _read_model( alias=manifest_model.get( "alias", manifest_model.get("identifier", manifest_model["name"]) ), - description=scanned_fields.get( - "description", manifest_model.get("description") - ), + description=description, columns=columns, unique_id=unique_id, source=source,