Skip to content
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

Re-enable pylint for superset/utils folder #8766

Merged
merged 3 commits into from
Dec 6, 2019
Merged

Re-enable pylint for superset/utils folder #8766

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
cf8cf2a
re-enable pylint for superset/utils/import_datasource.py
Dec 4, 2019
1e623a6
re-enable pylint for superset/utils/cache.py
Dec 4, 2019
e5b48eb
re-enable pylint for superset/utils/log.py
Dec 4, 2019
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
8 changes: 2 additions & 6 deletions superset/utils/cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# pylint: disable=C,R,W
from typing import Optional

from flask import Flask, request
from flask_caching import Cache
from flask import request

from superset.extensions import cache_manager


def view_cache_key(*unused_args, **unused_kwargs) -> str:
def view_cache_key(*_, **__) -> str:
args_hash = hash(frozenset(request.args.items()))
return "view/{}/{}".format(request.path, args_hash)

Expand Down
23 changes: 11 additions & 12 deletions superset/utils/import_datasource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# pylint: disable=C,R,W
import logging

from sqlalchemy.orm.session import make_transient
Expand All @@ -30,7 +29,7 @@ def import_datasource(
superset instances. Audit metadata isn't copies over.
"""
make_transient(i_datasource)
logging.info("Started import of the datasource: {}".format(i_datasource.to_json()))
logging.info("Started import of the datasource: %s", i_datasource.to_json())
Copy link
Member

Choose a reason for hiding this comment

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

Can we use f strings instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

not according to pylint.

Copy link
Member

Choose a reason for hiding this comment

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

fstrings and .format are not recommended for logging statements because they make you pay the cost of formatting the string regardless of logging level. Not that it's expensive, but it's best practice to use the format above for that reason


i_datasource.id = None
i_datasource.database_id = lookup_database(i_datasource).id
Expand All @@ -47,25 +46,25 @@ def import_datasource(
session.add(datasource)
session.flush()

for m in i_datasource.metrics:
new_m = m.copy()
for metric in i_datasource.metrics:
new_m = metric.copy()
new_m.table_id = datasource.id
logging.info(
"Importing metric {} from the datasource: {}".format(
new_m.to_json(), i_datasource.full_name
)
"Importing metric %s from the datasource: %s",
new_m.to_json(),
i_datasource.full_name,
)
imported_m = i_datasource.metric_class.import_obj(new_m)
if imported_m.metric_name not in [m.metric_name for m in datasource.metrics]:
datasource.metrics.append(imported_m)

for c in i_datasource.columns:
new_c = c.copy()
for column in i_datasource.columns:
new_c = column.copy()
new_c.table_id = datasource.id
logging.info(
"Importing column {} from the datasource: {}".format(
new_c.to_json(), i_datasource.full_name
)
"Importing column %s from the datasource: %s",
new_c.to_json(),
i_datasource.full_name,
)
imported_c = i_datasource.column_class.import_obj(new_c)
if imported_c.column_name not in [c.column_name for c in datasource.columns]:
Expand Down
25 changes: 14 additions & 11 deletions superset/utils/log.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# pylint: disable=C,R,W
import functools
import inspect
import json
Expand Down Expand Up @@ -64,7 +63,7 @@ def wrapper(*args, **kwargs):
try:
explode_by = d.get("explode")
records = json.loads(d.get(explode_by))
except Exception:
except Exception: # pylint: disable=broad-except
records = [d]

referrer = request.referrer[:1000] if request.referrer else None
Expand All @@ -89,8 +88,9 @@ def stats_logger(self):

def get_event_logger_from_cfg_value(cfg_value: object) -> AbstractEventLogger:
"""
This function implements the deprecation of assignment of class objects to EVENT_LOGGER
configuration, and validates type of configured loggers.
This function implements the deprecation of assignment
of class objects to EVENT_LOGGER configuration, and validates
type of configured loggers.

The motivation for this method is to gracefully deprecate the ability to configure
EVENT_LOGGER with a class type, in favor of preconfigured instances which may have
Expand All @@ -105,10 +105,12 @@ def get_event_logger_from_cfg_value(cfg_value: object) -> AbstractEventLogger:
logging.warning(
textwrap.dedent(
"""
In superset private config, EVENT_LOGGER has been assigned a class object. In order to
accomodate pre-configured instances without a default constructor, assignment of a class
is deprecated and may no longer work at some point in the future. Please assign an object
instance of a type that implements superset.utils.log.AbstractEventLogger.
In superset private config, EVENT_LOGGER has been assigned a class
object. In order to accomodate pre-configured instances without a
default constructor, assignment of a class is deprecated and may no
longer work at some point in the future. Please assign an object
instance of a type that implements
superset.utils.log.AbstractEventLogger.
"""
)
)
Expand All @@ -119,15 +121,16 @@ def get_event_logger_from_cfg_value(cfg_value: object) -> AbstractEventLogger:
# Verify that we have a valid logger impl
if not isinstance(result, AbstractEventLogger):
raise TypeError(
"EVENT_LOGGER must be configured with a concrete instance of superset.utils.log.AbstractEventLogger."
"EVENT_LOGGER must be configured with a concrete instance"
"of superset.utils.log.AbstractEventLogger."
)

logging.info(f"Configured event logger of type {type(result)}")
return cast(AbstractEventLogger, result)


class DBEventLogger(AbstractEventLogger):
def log(self, user_id, action, *args, **kwargs):
def log(self, user_id, action, *args, **kwargs): # pylint: disable=too-many-locals
from superset.models.core import Log

records = kwargs.get("records", list())
Expand All @@ -140,7 +143,7 @@ def log(self, user_id, action, *args, **kwargs):
for record in records:
try:
json_string = json.dumps(record)
except Exception:
except Exception: # pylint: disable=broad-except
json_string = None
log = Log(
action=action,
Expand Down