style(mypy): Enforcing typing for superset.views module #9939

john-bodley · 2020-05-28T19:05:14Z

SUMMARY

Adding the mypy type enforcement for the remainder of the superset.views module. Note this PR is somewhat hairier that the previous versions, in part because of the brittle nature of many of the base views.

Note there were many views which were using the request.args.get(...) pattern meaning the result was of type Optional[Any] which would actually throw an exception if the result was None for functions like json.loads(...). Given that these functions aren't throwing I opted to use request.args[...] where possible as there seems to be an implicit definition that these values are always defined.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI.

ADDITIONAL INFORMATION

john-bodley · 2020-05-28T19:05:54Z

superset/connectors/base/models.py

-    metric_class: Optional[Type] = None  # link to derivative of BaseMetric
+
+    @property
+    def column_class(self) -> Type:


This is done to ensure that the column_class etc. is not-optional.

john-bodley · 2020-05-28T19:06:55Z

superset/views/core.py

-        form_data=form_data,
-        force=False,
-    )
+    if slc:


There's a number of places where slc could be None.

john-bodley · 2020-05-28T19:07:54Z

superset/views/core.py

        """Save or overwrite a slice"""
-        slice_name = args.get("slice_name")
-        action = args.get("action")
+        slice_name = request.args.get("slice_name")


No need to parse args which was merely request.args when request is global.

john-bodley · 2020-05-28T19:08:59Z

superset/views/core.py

@@ -2071,9 +2094,6 @@ def sqllab_viz(self):
        cols = []
        for config in data.get("columns"):
            column_name = config.get("name")
-            SqlaTable = ConnectorRegistry.sources["table"]


This abstraction seemed necessary given we're restricting the models to the SQLA connector.

codecov-commenter · 2020-05-28T19:11:48Z

Codecov Report

Merging #9939 into master will increase coverage by 0.00%.
The diff coverage is 85.27%.

@@           Coverage Diff           @@
##           master    #9939   +/-   ##
=======================================
  Coverage   71.39%   71.39%           
=======================================
  Files         585      585           
  Lines       30968    30982   +14     
  Branches     3261     3261           
=======================================
+ Hits        22109    22121   +12     
- Misses       8750     8752    +2     
  Partials      109      109

Flag	Coverage Δ
#cypress	`54.00% <ø> (-0.08%)`	⬇️
#javascript	`59.39% <ø> (ø)`
#python	`71.61% <85.27%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
superset/sql_validators/presto_db.py	`82.89% <ø> (ø)`
superset/views/base_schemas.py	`0.00% <0.00%> (ø)`
superset/viz.py	`71.94% <ø> (ø)`
superset/viz_sip38.py	`0.00% <0.00%> (ø)`
superset/connectors/base/models.py	`88.92% <66.66%> (-0.89%)`	⬇️
superset/views/api.py	`66.66% <75.00%> (+0.95%)`	⬆️
superset/views/schedules.py	`60.86% <83.33%> (-0.04%)`	⬇️
superset/views/core.py	`76.49% <86.16%> (-0.11%)`	⬇️
superset/views/tags.py	`40.74% <88.88%> (+1.50%)`	⬆️
superset/connectors/druid/models.py	`82.47% <100.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 244677c...dfee72a. Read the comment docs.

john-bodley · 2020-05-28T19:39:04Z

superset/views/core.py

@@ -1831,15 +1858,15 @@ def publish(self, dashboard_id):
            return json_success(json.dumps({"published": dash.published}))

    @has_access
-    @expose("/dashboard/<dashboard_id>/")
-    def dashboard(self, dashboard_id):
+    @expose("/dashboard/<dashboard_id_or_slug>/")


dashboard_id_or_slug seemed to be a more accurate representation of the field.

john-bodley · 2020-06-01T20:30:32Z

@villebro et al. would you mind taking a look at this PR?

serenajiang

what a beauty.

a few small comments, but otherwise LGTM.

superset/sql_validators/presto_db.py

superset/views/base.py

superset/viz.py

serenajiang · 2020-06-02T17:28:56Z

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:


Maybe we can be a bit more precise about Callable throughout? Something like Callable[..., FlaskResponse]?

Across the code base Callable is one type which we haven't been more explicit about in terms of arguments and the return type. I wonder for simplicity whether these can remain Callable and in a later PR add the return type to all Callable types.

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.

villebro

First pass comments. If all my comments are irrelevant, I'm fine merging as-is 👍

villebro · 2020-06-02T20:29:45Z

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:


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.

villebro · 2020-06-02T20:48:50Z

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


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

@villebro this is part of the complexity I discussed during the meetup lastweek here, i.e., complex logic leads to a number of states which aren't trivial to digest without thoroughly reading through the code.

Under certain circumstances datasource_type can be None however in this context it will never to None hence the reason for the cast(...) which per the documentation states,

... give the type checker a little help when it can’t quite understand what is going on.

Ok, makes sense.

villebro · 2020-06-02T20:50:44Z

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


Same as above

villebro · 2020-06-02T20:50:54Z

superset/views/core.py

                slc,
                slice_add_perm,
                slice_overwrite_perm,
                slice_download_perm,
                datasource_id,
-                datasource_type,
+                cast(str, datasource_type),


villebro · 2020-06-02T20:54:56Z

superset/views/core.py

+            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(),


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

@villebro for some reason it think's it's an Optional[Dashboard] (possibly due to the type declaration on line 1040).

john-bodley · 2020-06-03T23:49:36Z

Thanks @serenajiang and @villebro for the review. I believe I've addressed all your comments and thus this is ready for re-review.

Co-authored-by: John Bodley <john.bodley@airbnb.com>

Co-authored-by: John Bodley <john.bodley@airbnb.com> (cherry picked from commit 36627af)

Co-authored-by: John Bodley <john.bodley@airbnb.com>

pull-request-size bot added the size/XL label May 28, 2020

john-bodley commented May 28, 2020

View reviewed changes

john-bodley requested review from dpgaspar, villebro and mistercrunch May 28, 2020 19:11

john-bodley force-pushed the john-bodley--mypy-enforcement-views-remainder branch 2 times, most recently from f0670ef to 07d9986 Compare May 28, 2020 19:37

john-bodley commented May 28, 2020

View reviewed changes

john-bodley force-pushed the john-bodley--mypy-enforcement-views-remainder branch from 07d9986 to f3129b7 Compare May 28, 2020 20:02

john-bodley mentioned this pull request May 29, 2020

style(mypy): Enforcing typing for superset module #9943

Merged

6 tasks

john-bodley changed the title ~~style(mypy): Enforcing typing for superset.views~~ style(mypy): Enforcing typing for superset.views module May 29, 2020

serenajiang reviewed Jun 2, 2020

View reviewed changes

villebro reviewed Jun 2, 2020

View reviewed changes

john-bodley force-pushed the john-bodley--mypy-enforcement-views-remainder branch 5 times, most recently from dfe196c to 900a03b Compare June 3, 2020 22:59

style(mypy): Enforcing typing for superset.views

dfee72a

john-bodley force-pushed the john-bodley--mypy-enforcement-views-remainder branch from 900a03b to dfee72a Compare June 3, 2020 23:31

john-bodley requested a review from villebro June 4, 2020 15:51

john-bodley merged commit 63e0188 into apache:master Jun 5, 2020

john-bodley deleted the john-bodley--mypy-enforcement-views-remainder branch June 5, 2020 15:44

john-bodley added a commit to john-bodley/superset that referenced this pull request Jun 10, 2020

fix: Resolves a regression from apache#9939

3a22562

john-bodley mentioned this pull request Jun 10, 2020

fix: Resolves a regression from #9939 #10033

Merged

6 tasks

john-bodley added a commit that referenced this pull request Jun 10, 2020

fix: Resolves a regression from #9939 (#10033)

36627af

Co-authored-by: John Bodley <john.bodley@airbnb.com>

graceguo-supercat pushed a commit to airbnb/superset-fork that referenced this pull request Jun 10, 2020

fix: Resolves a regression from apache#9939 (apache#10033)

316709b

Co-authored-by: John Bodley <john.bodley@airbnb.com> (cherry picked from commit 36627af)

nytai mentioned this pull request Jun 16, 2020

fix: use custom int parsing over flask int parsing in sqllab queries endpoint #10070

Merged

6 tasks

dpgaspar mentioned this pull request Jul 1, 2020

fix: dashboard endpoint sig changed #10220

Merged

6 tasks

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

style(mypy): Enforcing typing for superset.views (apache#9939)

9d97ee8

Co-authored-by: John Bodley <john.bodley@airbnb.com>

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

fix: Resolves a regression from apache#9939 (apache#10033)

56b8d4b

Co-authored-by: John Bodley <john.bodley@airbnb.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(mypy): Enforcing typing for superset.views module #9939

style(mypy): Enforcing typing for superset.views module #9939

john-bodley commented May 28, 2020 •

edited

john-bodley May 28, 2020

john-bodley May 28, 2020

john-bodley May 28, 2020

john-bodley May 28, 2020

codecov-commenter commented May 28, 2020 •

edited

john-bodley May 28, 2020

john-bodley commented Jun 1, 2020 •

edited

serenajiang left a comment

serenajiang Jun 2, 2020

john-bodley Jun 2, 2020 •

edited

villebro Jun 2, 2020

villebro left a comment

villebro Jun 2, 2020

villebro Jun 2, 2020

john-bodley Jun 2, 2020 •

edited

villebro Jun 2, 2020

villebro Jun 2, 2020

villebro Jun 2, 2020

villebro Jun 2, 2020

john-bodley Jun 3, 2020

john-bodley commented Jun 3, 2020

style(mypy): Enforcing typing for superset.views module #9939

style(mypy): Enforcing typing for superset.views module #9939

Conversation

john-bodley commented May 28, 2020 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 28, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

john-bodley commented Jun 1, 2020 • edited

serenajiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Jun 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Jun 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Jun 3, 2020

john-bodley commented May 28, 2020 •

edited

codecov-commenter commented May 28, 2020 •

edited

john-bodley commented Jun 1, 2020 •

edited

john-bodley Jun 2, 2020 •

edited

john-bodley Jun 2, 2020 •

edited