Re: [PR] fix: Dataset currency [superset]

[via GitHub]

korbit-ai[bot] commented on code in PR #33682:
URL: https://github.com/apache/superset/pull/33682#discussion_r2125646824


##########
superset/datasets/schemas.py:
##########
@@ -52,6 +59,181 @@ def validate_python_date_format(dt_format: str) -> bool:
     return True
 
 
+class UserSchema(Schema):
+    first_name = fields.String(metadata={"description": 
"first_name_description"})
+    last_name = fields.String(metadata={"description": 
"last_name_description"})
+
+
+class OwnerSchema(UserSchema):
+    id = fields.Integer(metadata={"description": "id_description"})
+
+
+class DatasetColumnsShowSchema(Schema):
+    advanced_data_type = fields.String(
+        metadata={"description": "advanced_data_type_description"}, 
allow_none=True
+    )
+    changed_on = fields.DateTime(metadata={"description": 
"changed_on_description"})
+    column_name = fields.String(metadata={"description": 
"column_name_description"})
+    created_on = fields.DateTime(metadata={"description": 
"created_on_description"})
+    description = fields.String(
+        metadata={"description": "description_description"}, allow_none=True
+    )
+    expression = fields.String(metadata={"description": 
"expression_description"})
+    extra = fields.String(metadata={"description": "extra_description"})
+    filterable = fields.Boolean(metadata={"description": 
"filterable_description"})
+    groupby = fields.Boolean(metadata={"description": "groupby_description"})
+    id = fields.Integer(metadata={"description": "id_description"})
+    is_active = fields.Boolean(metadata={"description": 
"is_active_description"})
+    is_dttm = fields.Boolean(metadata={"description": "is_dttm_description"})
+    python_date_format = fields.String(
+        metadata={"description": "python_date_format_description"}, 
allow_none=True
+    )
+    type = fields.String(metadata={"description": "type_description"})
+    type_generic = fields.String(metadata={"description": 
"type_generic_description"})
+    uuid = fields.UUID(metadata={"description": "uuid_description"})
+    verbose_name = fields.String(
+        metadata={"description": "verbose_name_description"}, allow_none=True
+    )
+
+
+class DatasetDatabaseShowSchema(Schema):
+    allow_multi_catalog = fields.Boolean(
+        metadata={"description": "allow_multi_catalog_description"}
+    )
+    backend = fields.String(metadata={"description": "backend_description"})
+    database_name = fields.String(metadata={"description": 
"database_name_description"})
+    id = fields.Integer(metadata={"description": "id_description"})
+
+
+class DatasetMetricCurrencyShowSchema(Schema):
+    @pre_dump
+    def load_currency_config_from_string(  # pylint: disable=unused-argument
+        self,
+        data: dict[str, Any] | None,
+        **kwargs: Any,
+    ) -> dict[str, Any] | None:
+        if data and isinstance(data.get("currency"), str):
+            try:
+                data["currency"] = json.loads(data["currency"])
+            except ValueError:
+                data["currency"] = None
+        return data

Review Comment:
   ### Misleading Method Name in Currency Schema <sub>![category 
Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The method name 'load_currency_config_from_string' is misleading since it's 
used in a @pre_dump decorator which runs before serialization (dumping), not 
during loading.
   
   
   ###### Why this matters
   The misleading method name could cause confusion about when this 
transformation occurs in the data flow, potentially leading to incorrect 
assumptions about data state during development and debugging.
   
   ###### Suggested change ∙ *Feature Preview*
   Rename the method to better reflect its purpose in the serialization process:
   ```python
   @pre_dump
   def transform_currency_for_serialization(
       self,
       data: dict[str, Any] | None,
       **kwargs: Any,
   ) -> dict[str, Any] | None:
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b11e762-5e60-4538-bac8-1aaf8eb6d10a/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b11e762-5e60-4538-bac8-1aaf8eb6d10a?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b11e762-5e60-4538-bac8-1aaf8eb6d10a?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b11e762-5e60-4538-bac8-1aaf8eb6d10a?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b11e762-5e60-4538-bac8-1aaf8eb6d10a)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:6a8b2bf8-4139-4042-b0f6-455b4a12d6d1 -->
   
   
   [](6a8b2bf8-4139-4042-b0f6-455b4a12d6d1)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org