style(mypy): Enforcing typing for superset module #9943
Conversation
| @@ -53,7 +53,7 @@ order_by_type = false | |||
| ignore_missing_imports = true | |||
| no_implicit_optional = true | |||
|
|
|||
| [mypy-superset.bin.*,superset.charts.*,superset.commands.*,superset.common.*,superset.connectors.*,superset.dao.*,superset.dashboards.*,superset.datasets.*,superset.db_engine_specs.*,superset.db_engines.*,superset.examples.*,superset.migrations.*,superset.models.*,uperset.queries.*,superset.security.*,superset.sql_validators.*,superset.tasks.*,superset.translations.*,superset.utils.*,superset.views.chart.*,superset.views.dashboard.*,superset.views.database.*] | |||
| [mypy-superset,superset.app,superset.bin.*,superset.charts.*,superset.cli,superset.commands.*,superset.common.*,superset.config,superset.connectors.*,superset.constants,superset.dataframe,superset.dao.*,superset.dashboards.*,superset.datasets.*,superset.db_engine_specs.*,superset.db_engines.*,superset.errors,superset.examples.*,superset.exceptions,superset.extensions,superset.forms,superset.jinja_context,superset.legacy,superset.migrations.*,superset.models.*,superset.result_set,superset.queries.*,superset.security.*,superset.sql_lab,superset.sql_parse,superset.sql_validators.*,superset.stats_logger,superset.tasks.*,superset.translations.*,superset.typing,superset.utils.*,superset.views.chart.*,superset.views.dashboard.*,superset.views.database.*,superset.viz,superset.viz_sip38] | |||
There was a problem hiding this comment.
Note after this PR and #9939 are merged this should be updated to read,
[mypy-superset.*]
john-bodley
force-pushed
the
john-bodley--mypy-enforcement-remainder
branch
from
20e76e1 to
9a5b77f
Compare
Codecov Report
@@ Coverage Diff @@
## master #9943 +/- ##
==========================================
+ Coverage 71.31% 71.37% +0.05%
==========================================
Files 585 585
Lines 30887 30933 +46
Branches 3236 3246 +10
==========================================
+ Hits 22027 22078 +51
+ Misses 8750 8746 -4
+ Partials 110 109 -1
Continue to review full report at Codecov.
|
john-bodley
force-pushed
the
john-bodley--mypy-enforcement-remainder
branch
5 times, most recently
from
374aeca to
bb4eb34
Compare
john-bodley
changed the title
|
@villebro et al. would you mind taking a look at this PR? |
| def __call__(self, *args, **kwargs): | ||
| with flask_app.app_context(): | ||
| def __call__(self, *args: Any, **kwargs: Any) -> Any: | ||
| with flask_app.app_context(): # type: ignore |
There was a problem hiding this comment.
This probably is for later, we should look into using the flask stubs in typeshed
There was a problem hiding this comment.
@serenajiang mypy does use these stubs. I presume the Flask types aren't fully flushed out. I plan on creating a follow up PR which will check for erroneous # type: ignore comments which then can be removed as mypy continues to evolve.
superset/viz.py
Outdated
| @@ -559,7 +563,7 @@ class TableViz(BaseViz): | |||
| is_timeseries = False | |||
| enforce_numerical_metrics = False | |||
|
|
|||
| def should_be_timeseries(self): | |||
| def should_be_timeseries(self) -> Optional[bool]: | |||
There was a problem hiding this comment.
In this case, if should_be_timeseries() is None, does that mean this is False? It could be good to cast the results to bool to avoid the weird sorts of results that occur with short circuiting + .get. Then, we can replace most of the Optional[bool] types with bool.
superset/viz.py
Outdated
| procs: Dict[int, pd.DataFrame], | ||
| level: int = -1, | ||
| dims: Optional[Tuple[str, ...]] = None, | ||
| time: Optional[Any] = None, |
There was a problem hiding this comment.
| time: Optional[Any] = None, | |
| time: Any = None, |
john-bodley
force-pushed
the
john-bodley--mypy-enforcement-remainder
branch
from
bb4eb34 to
6bcbf55
Compare
| @@ -2367,7 +2382,7 @@ def query_obj(self): | |||
| d["columns"] = gb | |||
| return d | |||
|
|
|||
| def get_js_columns(self, d): | |||
| def get_js_columns(self, d: Dict[str, Any]) -> Dict[str, Any]: | |||
There was a problem hiding this comment.
d looks like a bad variable name. I thought these were forbidden in .pylintrc?
There was a problem hiding this comment.
@villebro I'm not sure if you're aware, but numerous files have pylint checks and warnings (not errors) disabled including viz.py per the line:
# pylint: disable=C,R,W
Note the plan is to address this in SIP-46. I'm not quite mentally prepared for that saga yet as the typing work has left me a tad battered and bruised.
There was a problem hiding this comment.
😄 I'm sure it has, kudos for this work! I think we need to bump pylint first, as the current version isn't compatible with 3.7. I'm looking to see if there's interest in the community to help with that effort, as it might be a good task to get acquainted with the codebase.
There was a problem hiding this comment.
Agreed regarding bumping pylint (this is actually mentioned in the SIP) as I ran into an issue trying to use a newer feature. I'm not sure if we can automate this, but periodically I think it's good that we go through and bump all our dependencies which should help us be more nimble to accommodate us supporting more recent Python versions.
Co-authored-by: John Bodley <[email protected]>
mistercrunch
added
🏷️ bot
SUMMARY
Yay! After starting this endeavor over three months ago with #9138, this PR adds
mypytype enforcement for the remainder of thesupersetpackage, thus signifying the final typing PR (for now at least).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI.
ADDITIONAL INFORMATION