Skip to content

chore: bump black to 19.10b0 and mypy to 0.770 #9378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
john-bodley merged 3 commits into from
Apr 4, 2020
Merged

chore: bump black to 19.10b0 and mypy to 0.770 #9378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ca8de7
Bump black to 19.10b0
ktmud Mar 25, 2020
9748812
Upgrade mypy to 0.770
ktmud Mar 25, 2020
fdff628
Update how inline type is defined
ktmud Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#
repos:
- repo: https://github.com/ambv/black
rev: 19.3b0
rev: 19.10b0
hooks:
- id: black
language_version: python3
Expand Down
4 changes: 2 additions & 2 deletions requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
black==19.3b0
black==19.10b0
coverage==4.5.3
flask-cors==3.0.7
flask-testing==0.7.1
ipdb==0.12
isort==4.3.21
mypy==0.670
mypy==0.770
nose==1.3.7
pip-tools==4.5.1
pre-commit==1.17.0
Expand Down
19 changes: 10 additions & 9 deletions superset/connectors/druid/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ def granularity(
"year": "P1Y",
}

granularity = {"type": "period"}
granularity: Dict[str, Union[str, float]] = {"type": "period"}
if timezone:
granularity["timeZone"] = timezone

Expand All @@ -831,7 +831,7 @@ def granularity(
granularity["period"] = period_name
else:
granularity["type"] = "duration"
granularity["duration"] = ( # type: ignore
granularity["duration"] = (
utils.parse_human_timedelta(period_name).total_seconds() * 1000
)
return granularity
Expand Down Expand Up @@ -941,23 +941,24 @@ def metrics_and_post_aggs(
adhoc_agg_configs = []
postagg_names = []
for metric in metrics:
if utils.is_adhoc_metric(metric):
if isinstance(metric, dict) and utils.is_adhoc_metric(metric):
Copy link
Member

@john-bodley john-bodley Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this was necessary as it seems is_adhoc_metric(metric) can handle both a dict and str. I wonder in adding typing there, i.e.,

is_adhoc_metric(metric: Union[Dict[str, Any], str]) -> bool:

would remove the need for isinstance(metric, dict) which is unnecessary.

Copy link
Member Author

@ktmud ktmud Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and L946 are for function calls later in the current context. Union[Dict, str] was passed on to adhoc_agg_configs , saved_agg_names and saved_agg_names, but could not be accepted by get_aggregations() and resolve_postagg(), which only accept either just dict or just str.

Apparently mypy was not smart enough to figure out metric type just by the outcome of is_adhoc_metric().

adhoc_agg_configs.append(metric)
elif metrics_dict[metric].metric_type != POST_AGG_TYPE: # type: ignore
saved_agg_names.add(metric)
else:
postagg_names.append(metric)
elif isinstance(metric, str):
if metrics_dict[metric].metric_type != POST_AGG_TYPE:
saved_agg_names.add(metric)
else:
postagg_names.append(metric)
# Create the post aggregations, maintain order since postaggs
# may depend on previous ones
post_aggs: "OrderedDict[str, Postaggregator]" = OrderedDict()
visited_postaggs = set()
for postagg_name in postagg_names:
postagg = metrics_dict[postagg_name] # type: ignore
postagg = metrics_dict[postagg_name]
visited_postaggs.add(postagg_name)
DruidDatasource.resolve_postagg(
postagg, post_aggs, saved_agg_names, visited_postaggs, metrics_dict
)
aggs = DruidDatasource.get_aggregations( # type: ignore
aggs = DruidDatasource.get_aggregations(
metrics_dict, saved_agg_names, adhoc_agg_configs
)
return aggs, post_aggs
Expand Down
2 changes: 1 addition & 1 deletion superset/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

class CommaSeparatedListField(Field):
widget = BS3TextFieldWidget()
data = [] # type: List[str]
data: List[str] = []

def _value(self):
if self.data:
Expand Down
4 changes: 2 additions & 2 deletions superset/models/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ def get_all_table_names_in_schema(
self,
schema: str,
cache: bool = False,
cache_timeout: int = None,
cache_timeout: Optional[int] = None,
force: bool = False,
) -> List[utils.DatasourceName]:
"""Parameters need to be passed as keyword arguments.
Expand Down Expand Up @@ -492,7 +492,7 @@ def get_all_view_names_in_schema(
self,
schema: str,
cache: bool = False,
cache_timeout: int = None,
cache_timeout: Optional[int] = None,
force: bool = False,
) -> List[utils.DatasourceName]:
"""Parameters need to be passed as keyword arguments.
Expand Down
12 changes: 4 additions & 8 deletions superset/utils/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
from cryptography.hazmat.backends.openssl.x509 import _Certificate
from dateutil.parser import parse
from dateutil.relativedelta import relativedelta
from flask import current_app, flash, Flask, g, Markup, render_template
from flask import current_app, flash, g, Markup, render_template
from flask_appbuilder import SQLA
from flask_appbuilder.security.sqla.models import User
from flask_babel import gettext as __, lazy_gettext as _
Expand Down Expand Up @@ -1057,15 +1057,11 @@ def get_since_until(
else:
rel, num, grain = time_range.split()
if rel == "Last":
since = relative_start - relativedelta( # type: ignore
**{grain: int(num)}
)
since = relative_start - relativedelta(**{grain: int(num)}) # type: ignore
until = relative_end
else: # rel == 'Next'
since = relative_start
until = relative_end + relativedelta( # type: ignore
**{grain: int(num)}
)
until = relative_end + relativedelta(**{grain: int(num)}) # type: ignore
else:
since = since or ""
if since:
Expand Down Expand Up @@ -1184,7 +1180,7 @@ def parse_ssl_cert(certificate: str) -> _Certificate:
return x509.load_pem_x509_certificate(
certificate.encode("utf-8"), default_backend()
)
except ValueError as e:
except ValueError:
raise CertificateException("Invalid certificate")


Expand Down
5 changes: 4 additions & 1 deletion superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,10 @@ def check_datasource_perms(
except SupersetException as e:
raise SupersetSecurityException(str(e))

viz_obj = get_viz( # type: ignore
if datasource_type is None:
Copy link
Member

@john-bodley john-bodley Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the get_datasource_info(...) method from a typing perspective. The datasource_type will be non-None (the method raises otherwise) and thus I think assert datasource_type is suffice from a type checking standpoint.

Copy link
Member Author

@ktmud ktmud Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it doesn't hurt to give a more user-friendly error message, would it?

Copy link
Member Author

@ktmud ktmud Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_datasource_info(...) only raises on datasource_id is None, but not on datasource_type.

raise SupersetSecurityException("Could not determine datasource type")

viz_obj = get_viz(
datasource_type=datasource_type,
datasource_id=datasource_id,
form_data=form_data,
Expand Down
2 changes: 1 addition & 1 deletion superset/views/schedules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class EmailScheduleView(
): # pylint: disable=too-many-ancestors
include_route_methods = RouteMethod.CRUD_SET
_extra_data = {"test_email": False, "test_email_recipients": None}
schedule_type: Optional[Type] = None
schedule_type: Optional[str] = None
schedule_type_model: Optional[Type] = None

page_size = 20
Expand Down
5 changes: 1 addition & 4 deletions tests/access_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,10 +567,7 @@ def test_request_access(self):
self.get_resp(ACCESS_REQUEST.format("druid", druid_ds_4_id, "go"))
access_request4 = self.get_access_requests("gamma", "druid", druid_ds_4_id)

self.assertEqual(
access_request4.roles_with_datasource,
"<ul></ul>".format(access_request4.id),
)
self.assertEqual(access_request4.roles_with_datasource, "<ul></ul>")

# Case 5. Roles exist that contains the druid datasource.
# add druid ds to the existing roles
Expand Down
10 changes: 5 additions & 5 deletions tests/db_engine_specs/hive_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def test_job_1_launched_stage_1(self):
self.assertEqual(0, HiveEngineSpec.progress(log))

def test_job_1_launched_stage_1_map_40_progress(
self
self,
): # pylint: disable=invalid-name
log = """
17/02/07 19:15:55 INFO ql.Driver: Total jobs = 2
Expand All @@ -71,7 +71,7 @@ def test_job_1_launched_stage_1_map_40_progress(
self.assertEqual(10, HiveEngineSpec.progress(log))

def test_job_1_launched_stage_1_map_80_reduce_40_progress(
self
self,
): # pylint: disable=invalid-name
log = """
17/02/07 19:15:55 INFO ql.Driver: Total jobs = 2
Expand All @@ -85,7 +85,7 @@ def test_job_1_launched_stage_1_map_80_reduce_40_progress(
self.assertEqual(30, HiveEngineSpec.progress(log))

def test_job_1_launched_stage_2_stages_progress(
self
self,
): # pylint: disable=invalid-name
log = """
17/02/07 19:15:55 INFO ql.Driver: Total jobs = 2
Expand All @@ -101,7 +101,7 @@ def test_job_1_launched_stage_2_stages_progress(
self.assertEqual(12, HiveEngineSpec.progress(log))

def test_job_2_launched_stage_2_stages_progress(
self
self,
): # pylint: disable=invalid-name
log = """
17/02/07 19:15:55 INFO ql.Driver: Total jobs = 2
Expand Down Expand Up @@ -145,7 +145,7 @@ def test_hive_error_msg(self):
)

def test_hive_get_view_names_return_empty_list(
self
self,
): # pylint: disable=invalid-name
self.assertEqual(
[], HiveEngineSpec.get_view_names(mock.ANY, mock.ANY, mock.ANY)
Expand Down
2 changes: 1 addition & 1 deletion tests/db_engine_specs/presto_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def test_get_datatype_presto(self):
self.assertEqual("STRING", PrestoEngineSpec.get_datatype("string"))

def test_presto_get_view_names_return_empty_list(
self
self,
): # pylint: disable=invalid-name
self.assertEqual(
[], PrestoEngineSpec.get_view_names(mock.ANY, mock.ANY, mock.ANY)
Expand Down
3 changes: 2 additions & 1 deletion tests/superset_test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# type: ignore
Copy link
Member

@john-bodley john-bodley Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this change? It makes typing less strict.

Copy link
Member Author

@ktmud ktmud Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting tons of errors about variables from import * being overridden. Didn't find a way to bypass it and thought it wouldn't make much sense to type: ignore them one by one in a config file.

from copy import copy

from superset.config import * # type: ignore
from superset.config import *

AUTH_USER_REGISTRATION_ROLE = "alpha"
SQLALCHEMY_DATABASE_URI = "sqlite:///" + os.path.join(DATA_DIR, "unittests.db")
Expand Down
3 changes: 2 additions & 1 deletion tests/superset_test_config_sqllab_backend_persist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@
# specific language governing permissions and limitations
# under the License.
# flake8: noqa
# type: ignore
Copy link
Member

@john-bodley john-bodley Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See ^^.

import os
from copy import copy

from superset.config import * # type: ignore
from superset.config import *

AUTH_USER_REGISTRATION_ROLE = "alpha"
SQLALCHEMY_DATABASE_URI = "sqlite:///" + os.path.join(DATA_DIR, "unittests.db")
Expand Down