[GitHub] [incubator-superset] villebro commented on a change in pull request #9824: style(mypy): enforcing mypy typing for connectors

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

villebro commented on a change in pull request #9824:
URL: https://github.com/apache/incubator-superset/pull/9824#discussion_r426380594



##########
File path: superset/connectors/base/models.py
##########
@@ -182,7 +182,7 @@ def short_data(self) -> Dict[str, Any]:
         }
 
     @property
-    def select_star(self):
+    def select_star(self) -> str:
         pass

Review comment:
       This probably needs to be `Optional[str]` or alternatively raise `NotImplementedError`.

##########
File path: superset/typing.py
##########
@@ -16,15 +16,31 @@
 # under the License.
 from typing import Any, Callable, Dict, List, Optional, Tuple, Union
 
-from flask import Flask
+import flask
+import werkzeug
 from flask_caching import Cache
 
-CacheConfig = Union[Callable[[Flask], Cache], Dict[str, Any]]
+CacheConfig = Union[Callable[[flask.Flask], Cache], Dict[str, Any]]
 DbapiDescriptionRow = Tuple[
     str, str, Optional[str], Optional[str], Optional[int], Optional[int], bool
 ]
 DbapiDescription = Union[List[DbapiDescriptionRow], Tuple[DbapiDescriptionRow, ...]]
 DbapiResult = List[Union[List[Any], Tuple[Any, ...]]]
 FilterValue = Union[float, int, str]
 FilterValues = Union[FilterValue, List[FilterValue], Tuple[FilterValue]]
+Granularity = Union[str, Dict[str, Union[str, float]]]
+Metric = Union[Dict[str, str], str]
+QueryObject = Dict[str, Any]

Review comment:
       We should probably call this `QueryObjectDict` to distinguish from `superset.common.query_object.QueryObject`.

##########
File path: superset/connectors/base/models.py
##########
@@ -93,7 +93,7 @@ class BaseDatasource(
     update_from_object_fields: List[str]
 
     @declared_attr
-    def slices(self):
+    def slices(self) -> relationship:

Review comment:
       I believe this should be `RelationshipProperty`. See: https://docs.sqlalchemy.org/en/13/orm/relationship_api.html#sqlalchemy.orm.relationship

##########
File path: superset/connectors/druid/models.py
##########
@@ -558,8 +569,8 @@ def get_perm(self) -> str:
             obj=self
         )
 
-    def update_from_object(self, obj):
-        return NotImplementedError()

Review comment:
       😄 

##########
File path: superset/connectors/druid/models.py
##########
@@ -1322,13 +1347,13 @@ def run_query(  # druid
                     set([x for x in pre_qry_dims if not isinstance(x, dict)])
                 )
                 dict_dims = [x for x in pre_qry_dims if isinstance(x, dict)]
-                pre_qry["dimensions"] = non_dict_dims + dict_dims
+                pre_qry["dimensions"] = non_dict_dims + dict_dims  # type: ignore
 
-                order_by = None
+                order_by = None  # type: ignore

Review comment:
       If we define this as `order_by: Optional[Metric]`, we can probably omit the ignore?

##########
File path: superset/connectors/druid/models.py
##########
@@ -817,7 +829,7 @@ def granularity(
             "year": "P1Y",
         }
 
-        granularity: Dict[str, Union[str, float]] = {"type": "period"}
+        granularity = {"type": "period"}

Review comment:
       I would have thought this turns into a `Dict[str, str]` unless being explicitly being defined as `Granularity`, and thus causing problems below when numeric values are added? 

##########
File path: superset/connectors/druid/models.py
##########
@@ -449,7 +460,7 @@ def get_perm(self) -> Optional[str]:
 
     @classmethod
     def import_obj(cls, i_metric: "DruidMetric") -> "DruidMetric":
-        def lookup_obj(lookup_metric: DruidMetric) -> Optional[DruidMetric]:
+        def lookup_obj(lookup_metric: "DruidMetric") -> Optional["DruidMetric"]:

Review comment:
       Not sure how a nested function referencing its parent method's class is handled by mypy, but if this was working before, we should probably be ok leaving this without quotes?

##########
File path: superset/connectors/druid/models.py
##########
@@ -1240,7 +1268,7 @@ def run_query(  # druid
             qry["limit"] = row_limit
             client.scan(**qry)
         elif (IS_SIP_38 and columns) or (
-            not IS_SIP_38 and len(groupby) == 0 and not having_filters
+            not IS_SIP_38 and (not groupby or len(groupby) == 0) and not having_filters

Review comment:
       I wonder if a simple falsy check here would be more pythonic: `not groupby`? I can't think of an edge case for which it wouldn't give the correct results.




----------------------------------------------------------------
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.

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