[GitHub] [incubator-superset] villebro commented on a change in pull request #9939: style(mypy): Enforcing typing for superset.views module

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

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



##########
File path: superset/views/base.py
##########
@@ -154,14 +157,14 @@ def wraps(self, *args, **kwargs):
     return functools.update_wrapper(wraps, f)
 
 
-def handle_api_exception(f):
+def handle_api_exception(f: Callable) -> Callable:

Review comment:
       I think it's probably a good idea to add the full signature where it is simple, and leave as just `Callable` if it would require extensive research or unions.

##########
File path: superset/views/core.py
##########
@@ -834,7 +861,7 @@ def explore(self, datasource_type=None, datasource_id=None):
             return redirect(error_redirect)
 
         datasource = ConnectorRegistry.get_datasource(
-            datasource_type, datasource_id, db.session
+            cast(str, datasource_type), datasource_id, db.session

Review comment:
       Same as above

##########
File path: superset/views/core.py
##########
@@ -744,7 +769,7 @@ def explore_json(self, datasource_type=None, datasource_id=None):
             )
 
             viz_obj = get_viz(
-                datasource_type=datasource_type,
+                datasource_type=cast(str, datasource_type),

Review comment:
       does this really need to be `cast` to `str`? I'm assuming this is always non-null.

##########
File path: superset/views/core.py
##########
@@ -1002,18 +1032,20 @@ def save_or_overwrite_slice(
         slc.datasource_id = datasource_id
         slc.slice_name = slice_name
 
-        if action in ("saveas") and slice_add_perm:
+        if action == "saveas" and slice_add_perm:
             self.save_slice(slc)
         elif action == "overwrite" and slice_overwrite_perm:
             self.overwrite_slice(slc)
 
         # Adding slice to a dashboard if requested
-        dash = None
+        dash: Optional[Dashboard] = None
+
         if request.args.get("add_to_dash") == "existing":
-            dash = (
+            dash = cast(
+                Dashboard,
                 db.session.query(Dashboard)
-                .filter_by(id=int(request.args.get("save_to_dashboard_id")))
-                .one()
+                .filter_by(id=int(request.args["save_to_dashboard_id"]))
+                .one(),

Review comment:
       Does this really need to be cast? I thought `one()` knows it will always be `Dashboard`.

##########
File path: superset/views/core.py
##########
@@ -890,14 +917,16 @@ def explore(self, datasource_type=None, datasource_id=None):
             )
 
         if action in ("saveas", "overwrite"):
+            if not slc:
+                return json_error_response("The slice does not exist")
+
             return self.save_or_overwrite_slice(
-                request.args,
                 slc,
                 slice_add_perm,
                 slice_overwrite_perm,
                 slice_download_perm,
                 datasource_id,
-                datasource_type,
+                cast(str, datasource_type),

Review comment:
       and here




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