[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9912: style(mypy): Enforcing mypy typing for views.chart

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

john-bodley commented on a change in pull request #9912:
URL: https://github.com/apache/incubator-superset/pull/9912#discussion_r430612963



##########
File path: superset/views/chart/views.py
##########
@@ -40,22 +41,22 @@ class SliceModelView(
         RouteMethod.API_DELETE,
     }
 
-    def pre_add(self, item):
+    def pre_add(self, item: "SliceModelView") -> None:
         utils.validate_json(item.params)
 
-    def pre_update(self, item):
+    def pre_update(self, item: "SliceModelView") -> None:
         utils.validate_json(item.params)
         check_ownership(item)
 
-    def pre_delete(self, item):
+    def pre_delete(self, item: "SliceModelView") -> None:
         check_ownership(item)
 
     @expose("/add", methods=["GET", "POST"])
     @has_access
-    def add(self):
-        datasources = ConnectorRegistry.get_all_datasources(db.session)
+    def add(self) -> FlaskResponse:
         datasources = [
-            {"value": str(d.id) + "__" + d.type, "label": repr(d)} for d in datasources
+            {"value": str(d.id) + "__" + str(d.type), "label": repr(d)}

Review comment:
       Note I need to wrap `d.type` in a `str(...)` as this can be optionally `None` (for right or wrong) per [here](https://github.com/apache/incubator-superset/blob/e5f5eed42504185723b3caf5b2042db333de32c7/superset/connectors/base/models.py#L41).

##########
File path: superset/views/chart/filters.py
##########
@@ -14,14 +14,17 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+from typing import Callable
+
 from sqlalchemy import or_
+from sqlalchemy.orm.query import Query
 
 from superset import security_manager
 from superset.views.base import BaseFilter
 
 
 class SliceFilter(BaseFilter):  # pylint: disable=too-few-public-methods
-    def apply(self, query, value):
+    def apply(self, query: Query, value: str) -> Query:

Review comment:
       @dpgaspar I'm not sure what the type of `value` should be. Looking at FAB I've seen it used as if it were a `str`. Additionally I've seen other causes where `value` is `func` so I'm not sure what's right here.

##########
File path: superset/views/chart/views.py
##########
@@ -40,22 +41,22 @@ class SliceModelView(
         RouteMethod.API_DELETE,
     }
 
-    def pre_add(self, item):
+    def pre_add(self, item: "SliceModelView") -> None:

Review comment:
       @dpgaspar is this correct?

##########
File path: superset/views/chart/filters.py
##########
@@ -14,14 +14,17 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+from typing import Callable
+
 from sqlalchemy import or_
+from sqlalchemy.orm.query import Query
 
 from superset import security_manager
 from superset.views.base import BaseFilter
 
 
 class SliceFilter(BaseFilter):  # pylint: disable=too-few-public-methods
-    def apply(self, query, value):
+    def apply(self, query: Query, value: str) -> Query:

Review comment:
       @dpgaspar I'm not sure what the type of `value` should be. Looking at FAB I've seen it used as if it were a `str`. Additionally I've seen other causes where `value` is named `func` so I'm not sure what the right name and type should be. I've also seen `None` used and thus I guess the `value` could/should be optional as well.




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