You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/06/19 05:53:55 UTC
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files
john-bodley commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r442643412
##########
File path: superset/connectors/sqla/views.py
##########
@@ -412,19 +425,19 @@ def edit(self, pk: int) -> FlaskResponse:
@action(
"refresh", __("Refresh Metadata"), __("Refresh column metadata"), "fa-refresh"
)
- def refresh(
+ def refresh( # pylint: disable=no-self-use
self, tables: Union["TableModelView", List["TableModelView"]]
) -> FlaskResponse:
if not isinstance(tables, list):
tables = [tables]
successes = []
failures = []
- for t in tables:
+ for my_table in tables:
Review comment:
See ^^.
##########
File path: superset/connectors/sqla/models.py
##########
@@ -924,9 +934,15 @@ def get_sqla_query( # sqla
elif op == utils.FilterOperator.LIKE.value:
where_clause_and.append(col_obj.get_sqla_col().like(eq))
elif op == utils.FilterOperator.IS_NULL.value:
- where_clause_and.append(col_obj.get_sqla_col() == None)
+ where_clause_and.append(
+ col_obj.get_sqla_col() # pylint: disable=singleton-comparison
Review comment:
I think `is None` and `is not None` will resolve lines 938 and 943 respectively.
##########
File path: superset/connectors/sqla/models.py
##########
@@ -598,18 +597,18 @@ def select_star(self) -> Optional[str]:
@property
def data(self) -> Dict[str, Any]:
- d = super().data
+ my_data = super().data
Review comment:
I'm not sure if `my_data` is a good variable name here, i.e., I don't understand the `my` prefix. Maybe `data_`?
##########
File path: superset/connectors/sqla/models.py
##########
@@ -560,8 +559,8 @@ def any_dttm_col(self) -> Optional[str]:
@property
def html(self) -> str:
- t = ((c.column_name, c.type) for c in self.columns)
- df = pd.DataFrame(t)
+ data_for_frame = ((c.column_name, c.type) for c in self.columns)
+ df = pd.DataFrame(data_for_frame)
Review comment:
Could lines 562 and 563 be combined,
```python
df = pd.DataFrame(((c.column_name, c.type) for c in self.columns))
```
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1152,7 +1172,7 @@ def get_sqla_table_object(self) -> Table:
def fetch_metadata(self, commit: bool = True) -> None:
"""Fetches the metadata for the table and merges it in"""
try:
- table = self.get_sqla_table_object()
+ my_table = self.get_sqla_table_object()
Review comment:
Per the previous comment would `table_` or similar be better?
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1169,24 +1189,30 @@ def fetch_metadata(self, commit: bool = True) -> None:
dbcols = (
db.session.query(TableColumn)
.filter(TableColumn.table == self)
- .filter(or_(TableColumn.column_name == col.name for col in table.columns))
+ .filter(
+ or_(TableColumn.column_name == col.name for col in my_table.columns)
+ )
)
dbcols = {dbcol.column_name: dbcol for dbcol in dbcols}
- for col in table.columns:
+ for col in my_table.columns:
try:
datatype = db_engine_spec.column_datatype_to_string(
col.type, db_dialect
)
- except Exception as ex:
+ except Exception as ex: # pylint: disable=broad-except
datatype = "UNKNOWN"
- logger.error("Unrecognized data type in {}.{}".format(table, col.name))
+ logger.error("Unrecognized data type in %s.%s", my_table, col.name)
logger.exception(ex)
dbcol = dbcols.get(col.name, None)
if not dbcol:
dbcol = TableColumn(column_name=col.name, type=datatype, table=self)
- dbcol.sum = dbcol.is_numeric
- dbcol.avg = dbcol.is_numeric
+ dbcol.sum = (
+ dbcol.is_numeric
+ ) # pylint: disable=attribute-defined-outside-init
+ dbcol.avg = (
+ dbcol.is_numeric
+ ) # pylint: disable=attribute-defined-outside-init
Review comment:
Yay `pylint` paying dividends. I think these could be annexed as they don't seem to be part of the `TableColumn` class nor seem to be used.
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1227,30 +1253,30 @@ def import_obj(
superset instances. Audit metadata isn't copies over.
"""
- def lookup_sqlatable(table: "SqlaTable") -> "SqlaTable":
+ def lookup_sqlatable(by_table: "SqlaTable") -> "SqlaTable":
Review comment:
Per the previous comment would `table_` or similar be better?
##########
File path: superset/security/manager.py
##########
@@ -889,9 +907,12 @@ def assert_viz_permission(self, viz: "BaseViz") -> None:
self.assert_datasource_permission(viz.datasource)
- def get_rls_filters(self, table: "BaseDatasource") -> List[Query]:
+ def get_rls_filters( # pylint: disable=no-self-use
+ self, table: "BaseDatasource"
+ ) -> List[Query]:
"""
- Retrieves the appropriate row level security filters for the current user and the passed table.
+ Retrieves the appropriate row level security
Review comment:
Any reason these aren't wrapped to the 88 character line limit?
##########
File path: superset/security/manager.py
##########
@@ -488,7 +499,7 @@ def get_schemas_accessible_by_user(
return [s for s in schemas if s in accessible_schemas]
- def get_datasources_accessible_by_user(
+ def datasources_accessible_by_user(
Review comment:
Per https://github.com/apache/incubator-superset/pull/10030 I think this should remain `get_datasources_accessible_by_user`.
----------------------------------------------------------------
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