[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #22304: feat: add Dataset tagging import/update api

[GitBox <gi...@apache.org>]

Antonio-RiveroMartnez commented on code in PR #22304:
URL: https://github.com/apache/superset/pull/22304#discussion_r1081697416


##########
superset/datasets/dao.py:
##########
@@ -167,6 +169,11 @@ def update(
         if "metrics" in properties:
             cls.update_metrics(model, properties.pop("metrics"), commit=commit)
 
+        if "tags" in properties and feature_flag_manager.is_feature_enabled(

Review Comment:
   same as above



##########
superset/datasets/commands/importers/v1/utils.py:
##########
@@ -148,6 +156,26 @@ def import_dataset(
     return dataset
 
 
+def extract_tags(config: Dict[str, Any]) -> Tuple[Dict[str, Any], Optional[List[str]]]:
+    tags = None
+    if "tags" in config.keys() and feature_flag_manager.is_feature_enabled(

Review Comment:
   I see a broader usage in our app of `is_feature_enabled` coming from this import directly `from superset import is_feature_enabled`, any reason why not using that instead?



##########
superset/datasets/commands/importers/v1/utils.py:
##########
@@ -148,6 +156,26 @@ def import_dataset(
     return dataset
 
 
+def extract_tags(config: Dict[str, Any]) -> Tuple[Dict[str, Any], Optional[List[str]]]:
+    tags = None
+    if "tags" in config.keys() and feature_flag_manager.is_feature_enabled(
+        "TAGGING_SYSTEM"
+    ):
+        tags = config.pop("tags")
+
+    return config, tags
+
+
+def import_tags(dataset: SqlaTable, existing: bool, tags: Optional[List[str]]) -> None:

Review Comment:
   Looking at this method, it's only doing something if `tags` has value in it, so why is `tags` `Optional` then? Can we just mark it as required and not call this method if tag if empty for example?



-- 
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-unsubscribe@superset.apache.org

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


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