From db6b2f3ae19740c86a537f031ec7fbee4b341742 Mon Sep 17 00:00:00 2001
From: Maxime Beauchemin <maximebeauchemin@gmail.com>
Date: Mon, 3 Apr 2017 21:53:06 -0700
Subject: [PATCH] pylint errors will now break the build (#2543)

* Linting pylint errors

* Backing off of an unecessary change
---
 .pylintrc                           |  2 +-
 .travis.yml                         |  1 +
 dev-reqs.txt                        |  3 +++
 pylint-errors.sh                    |  2 ++
 superset/cli.py                     |  8 +++----
 superset/connectors/base.py         | 29 ++++++++++++++++------
 superset/connectors/druid/models.py | 20 ++--------------
 superset/connectors/druid/views.py  |  7 +++---
 superset/connectors/sqla/models.py  | 19 ++++++++-------
 superset/data/__init__.py           |  4 ++--
 superset/db_engines/presto.py       |  2 ++
 superset/migrations/env.py          |  3 ++-
 superset/models/core.py             | 37 +++++++++++++++++------------
 superset/models/helpers.py          |  8 +++----
 superset/views/base.py              |  8 ++++---
 superset/views/core.py              |  6 +++--
 superset/viz.py                     |  2 +-
 tox.ini                             |  7 ++++++
 18 files changed, 99 insertions(+), 69 deletions(-)
 create mode 100755 pylint-errors.sh

diff --git a/.pylintrc b/.pylintrc
index 5a0ce1298ada3..ad87fcc21c076 100644
--- a/.pylintrc
+++ b/.pylintrc
@@ -277,7 +277,7 @@ ignore-mixin-members=yes
 # (useful for modules/projects where namespaces are manipulated during runtime
 # and thus existing member attributes cannot be deduced by static analysis. It
 # supports qualified module names, as well as Unix pattern matching.
-ignored-modules=
+ignored-modules=numpy,pandas,alembic.op,sqlalchemy,alembic.context,flask_appbuilder.security.sqla.PermissionView.role,flask_appbuilder.Model.metadata,flask_appbuilder.Base.metadata
 
 # List of class names for which member attributes should not be checked (useful
 # for classes with dynamically set attributes). This supports the use of
diff --git a/.travis.yml b/.travis.yml
index bd7bb7eb8ad0e..1ab01948ffb4f 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -17,6 +17,7 @@ env:
     - TRAVIS_NODE_VERSION="5.11"
   matrix:
     - TOX_ENV=javascript
+    - TOX_ENV=pylint
     - TOX_ENV=py34-postgres
     - TOX_ENV=py34-sqlite
     - TOX_ENV=py27-mysql
diff --git a/dev-reqs.txt b/dev-reqs.txt
index e6d8b0d224e1e..e3c2aa03f2f74 100644
--- a/dev-reqs.txt
+++ b/dev-reqs.txt
@@ -1,10 +1,13 @@
 codeclimate-test-reporter
 coveralls
 flake8
+flask_cors
 mock
 mysqlclient
 nose
 psycopg2
+pylint
+pythrifthiveapi
 pyyaml
 # Also install everything we need to build Sphinx docs
 -r dev-reqs-for-docs.txt
diff --git a/pylint-errors.sh b/pylint-errors.sh
new file mode 100755
index 0000000000000..5c2d022887c61
--- /dev/null
+++ b/pylint-errors.sh
@@ -0,0 +1,2 @@
+#!/bin/bash
+pylint superset --errors-only
diff --git a/superset/cli.py b/superset/cli.py
index dfeb3ae8aa5f4..b2a05e1379b71 100755
--- a/superset/cli.py
+++ b/superset/cli.py
@@ -133,8 +133,8 @@ def load_examples(load_test_data):
 def refresh_druid(datasource, merge):
     """Refresh druid datasources"""
     session = db.session()
-    from superset import models
-    for cluster in session.query(models.DruidCluster).all():
+    from superset.connectors.druid.models import DruidCluster
+    for cluster in session.query(DruidCluster).all():
         try:
             cluster.refresh_datasources(datasource_name=datasource,
                                         merge_flag=merge)
@@ -153,8 +153,8 @@ def refresh_druid(datasource, merge):
 @manager.command
 def update_datasources_cache():
     """Refresh sqllab datasources cache"""
-    from superset import models
-    for database in db.session.query(models.Database).all():
+    from superset.models.core import Database
+    for database in db.session.query(Database).all():
         print('Fetching {} datasources ...'.format(database.name))
         try:
             database.all_table_names(force=True)
diff --git a/superset/connectors/base.py b/superset/connectors/base.py
index 4cd20aeab9aad..c96f2bbb9969b 100644
--- a/superset/connectors/base.py
+++ b/superset/connectors/base.py
@@ -11,7 +11,12 @@ class BaseDatasource(AuditMixinNullable, ImportMixin):
 
     """A common interface to objects that are queryable (tables and datasources)"""
 
+    # ---------------------------------------------------------------
+    # class attributes to define when deriving BaseDatasource
+    # ---------------------------------------------------------------
     __tablename__ = None  # {connector_name}_datasource
+    type = None  # datasoure type, str to be defined when deriving this class
+    baselink = None  # url portion pointing to ModelView endpoint
 
     column_class = None  # link to derivative of BaseColumn
     metric_class = None  # link to derivative of BaseMetric
@@ -19,6 +24,10 @@ class BaseDatasource(AuditMixinNullable, ImportMixin):
     # Used to do code highlighting when displaying the query in the UI
     query_language = None
 
+    name = None  # can be a Column or a property pointing to one
+
+    # ---------------------------------------------------------------
+
     # Columns
     id = Column(Integer, primary_key=True)
     description = Column(Text)
@@ -30,6 +39,11 @@ class BaseDatasource(AuditMixinNullable, ImportMixin):
     params = Column(String(1000))
     perm = Column(String(1000))
 
+    # placeholder for a relationship to a derivative of BaseColumn
+    columns = []
+    # placeholder for a relationship to a derivative of BaseMetric
+    metrics = []
+
     @property
     def column_names(self):
         return sorted([c.column_name for c in self.columns])
@@ -69,6 +83,14 @@ def column_formats(self):
             if m.d3format
         }
 
+    @property
+    def metrics_combo(self):
+        return sorted(
+            [
+                (m.metric_name, m.verbose_name or m.metric_name)
+                for m in self.metrics],
+            key=lambda x: x[1])
+
     @property
     def data(self):
         """Data representation of the datasource sent to the frontend"""
@@ -91,13 +113,6 @@ def data(self):
             'type': self.type,
         }
 
-        # TODO move this block to SqlaTable.data
-        if self.type == 'table':
-            grains = self.database.grains() or []
-            if grains:
-                grains = [(g.name, g.name) for g in grains]
-            d['granularity_sqla'] = utils.choicify(self.dttm_cols)
-            d['time_grain_sqla'] = grains
         return d
 
 
diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py
index e8063d3f7ac67..61cb3f019f346 100644
--- a/superset/connectors/druid/models.py
+++ b/superset/connectors/druid/models.py
@@ -268,16 +268,6 @@ class DruidMetric(Model, BaseMetric):
         enable_typechecks=False)
     json = Column(Text)
 
-    def refresh_datasources(self, datasource_name=None, merge_flag=False):
-        """Refresh metadata of all datasources in the cluster
-
-        If ``datasource_name`` is specified, only that datasource is updated
-        """
-        self.druid_version = self.get_druid_version()
-        for datasource in self.get_datasources():
-            if datasource not in conf.get('DRUID_DATA_SOURCE_BLACKLIST'):
-                if not datasource_name or datasource_name == datasource:
-                    DruidDatasource.sync_to_db(datasource, self, merge_flag)
     export_fields = (
         'metric_name', 'verbose_name', 'metric_type', 'datasource_name',
         'json', 'description', 'is_restricted', 'd3format'
@@ -341,12 +331,6 @@ class DruidDatasource(Model, BaseDatasource):
         'cluster_name', 'offset', 'cache_timeout', 'params'
     )
 
-    @property
-    def metrics_combo(self):
-        return sorted(
-            [(m.metric_name, m.verbose_name) for m in self.metrics],
-            key=lambda x: x[1])
-
     @property
     def database(self):
         return self.cluster
@@ -784,7 +768,7 @@ def recursive_get_fields(_conf):
                         mconf.get('probabilities', ''),
                     )
                 elif mconf.get('type') == 'fieldAccess':
-                    post_aggs[metric_name] = Field(mconf.get('name'), '')
+                    post_aggs[metric_name] = Field(mconf.get('name'))
                 elif mconf.get('type') == 'constant':
                     post_aggs[metric_name] = Const(
                         mconf.get('value'),
@@ -792,7 +776,7 @@ def recursive_get_fields(_conf):
                     )
                 elif mconf.get('type') == 'hyperUniqueCardinality':
                     post_aggs[metric_name] = HyperUniqueCardinality(
-                        mconf.get('name'), ''
+                        mconf.get('name')
                     )
                 else:
                     post_aggs[metric_name] = Postaggregator(
diff --git a/superset/connectors/druid/views.py b/superset/connectors/druid/views.py
index 892fd0f1e8620..895069692ffe9 100644
--- a/superset/connectors/druid/views.py
+++ b/superset/connectors/druid/views.py
@@ -100,11 +100,12 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
     }
 
     def post_add(self, metric):
-        utils.init_metrics_perm(superset, [metric])
+        if metric.is_restricted:
+            security.merge_perm(sm, 'metric_access', metric.get_perm())
 
     def post_update(self, metric):
-        utils.init_metrics_perm(superset, [metric])
-
+        if metric.is_restricted:
+            security.merge_perm(sm, 'metric_access', metric.get_perm())
 
 appbuilder.add_view_no_menu(DruidMetricInlineView)
 
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index 9788647ca7649..92942603bd197 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -251,14 +251,6 @@ def html(self):
                 "dataframe table table-striped table-bordered "
                 "table-condensed"))
 
-    @property
-    def metrics_combo(self):
-        return sorted(
-            [
-                (m.metric_name, m.verbose_name or m.metric_name)
-                for m in self.metrics],
-            key=lambda x: x[1])
-
     @property
     def sql_url(self):
         return self.database.sql_url + "?table_name=" + str(self.table_name)
@@ -276,6 +268,17 @@ def get_col(self, col_name):
             if col_name == col.column_name:
                 return col
 
+    @property
+    def data(self):
+        d = super(SqlaTable, self).data
+        if self.type == 'table':
+            grains = self.database.grains() or []
+            if grains:
+                grains = [(g.name, g.name) for g in grains]
+            d['granularity_sqla'] = utils.choicify(self.dttm_cols)
+            d['time_grain_sqla'] = grains
+        return d
+
     def values_for_column(self, column_name, limit=10000):
         """Runs query against sqla to retrieve some
         sample values for the given column.
diff --git a/superset/data/__init__.py b/superset/data/__init__.py
index cac631d4eef22..87599c4f0662c 100644
--- a/superset/data/__init__.py
+++ b/superset/data/__init__.py
@@ -818,7 +818,7 @@ def load_unicode_test_data():
     # generate date/numeric data
     df['date'] = datetime.datetime.now().date()
     df['value'] = [random.randint(1, 100) for _ in range(len(df))]
-    df.to_sql(
+    df.to_sql(  # pylint: disable=no-member
         'unicode_test',
         db.engine,
         if_exists='replace',
@@ -953,7 +953,7 @@ def load_long_lat_data():
     pdf['date'] = datetime.datetime.now().date()
     pdf['occupancy'] = [random.randint(1, 6) for _ in range(len(pdf))]
     pdf['radius_miles'] = [random.uniform(1, 3) for _ in range(len(pdf))]
-    pdf.to_sql(
+    pdf.to_sql(  # pylint: disable=no-member
         'long_lat',
         db.engine,
         if_exists='replace',
diff --git a/superset/db_engines/presto.py b/superset/db_engines/presto.py
index e6e84b1aa1b7a..57d04601711a7 100644
--- a/superset/db_engines/presto.py
+++ b/superset/db_engines/presto.py
@@ -11,6 +11,8 @@ def cancel(self):
         return
 
     response = presto.requests.delete(self._nextUri)
+
+    # pylint: disable=no-member
     if response.status_code != presto.requests.codes.no_content:
         fmt = "Unexpected status code after cancel {}\n{}"
         raise presto.OperationalError(
diff --git a/superset/migrations/env.py b/superset/migrations/env.py
index 428e6e3b534bd..599bda71fa822 100755
--- a/superset/migrations/env.py
+++ b/superset/migrations/env.py
@@ -20,9 +20,10 @@
 # for 'autogenerate' support
 # from myapp import mymodel
 from flask import current_app
+
 config.set_main_option('sqlalchemy.url',
                        current_app.config.get('SQLALCHEMY_DATABASE_URI'))
-target_metadata = Base.metadata
+target_metadata = Base.metadata   # pylint: disable=no-member
 
 # other values from the config, defined by the needs of env.py,
 # can be acquired:
diff --git a/superset/models/core.py b/superset/models/core.py
index 5889c7576ff84..184eb72fe98da 100644
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -44,6 +44,7 @@
 from urllib import parse  # noqa
 
 config = app.config
+metadata = Model.metadata  # pylint: disable=no-member
 
 
 def set_related_perm(mapper, connection, target):  # noqa
@@ -80,7 +81,7 @@ class CssTemplate(Model, AuditMixinNullable):
     css = Column(Text, default='')
 
 
-slice_user = Table('slice_user', Model.metadata,
+slice_user = Table('slice_user', metadata,
                    Column('id', Integer, primary_key=True),
                    Column('user_id', Integer, ForeignKey('ab_user.id')),
                    Column('slice_id', Integer, ForeignKey('slices.id'))
@@ -121,26 +122,30 @@ def datasource(self):
     @datasource.getter
     @utils.memoized
     def get_datasource(self):
-        ds = db.session.query(
-            self.cls_model).filter_by(
-            id=self.datasource_id).first()
-        return ds
+        return (
+            db.session.query(self.cls_model)
+            .filter_by(id=self.datasource_id)
+            .first()
+        )
 
     @renders('datasource_name')
     def datasource_link(self):
+        # pylint: disable=no-member
         datasource = self.datasource
-        if datasource:
-            return self.datasource.link
+        return datasource.link if datasource else None
 
     @property
     def datasource_edit_url(self):
-        self.datasource.url
+        # pylint: disable=no-member
+        datasource = self.datasource
+        return datasource.url if datasource else None
 
     @property
     @utils.memoized
     def viz(self):
         d = json.loads(self.params)
         viz_class = viz_types[self.viz_type]
+        # pylint: disable=no-member
         return viz_class(self.datasource, form_data=d)
 
     @property
@@ -269,14 +274,14 @@ def import_obj(cls, slc_to_import, import_time=None):
 
 
 dashboard_slices = Table(
-    'dashboard_slices', Model.metadata,
+    'dashboard_slices', metadata,
     Column('id', Integer, primary_key=True),
     Column('dashboard_id', Integer, ForeignKey('dashboards.id')),
     Column('slice_id', Integer, ForeignKey('slices.id')),
 )
 
 dashboard_user = Table(
-    'dashboard_user', Model.metadata,
+    'dashboard_user', metadata,
     Column('id', Integer, primary_key=True),
     Column('user_id', Integer, ForeignKey('ab_user.id')),
     Column('dashboard_id', Integer, ForeignKey('dashboards.id'))
@@ -307,6 +312,7 @@ def __repr__(self):
 
     @property
     def table_names(self):
+        # pylint: disable=no-member
         return ", ".join(
             {"{}".format(s.datasource.full_name) for s in self.slices})
 
@@ -320,6 +326,7 @@ def datasources(self):
 
     @property
     def sqla_metadata(self):
+        # pylint: disable=no-member
         metadata = MetaData(bind=self.get_sqla_engine())
         return metadata.reflect()
 
@@ -816,7 +823,6 @@ class Query(Model):
     # Could be configured in the superset config.
     limit = Column(Integer)
     limit_used = Column(Boolean, default=False)
-    limit_reached = Column(Boolean, default=False)
     select_as_cta = Column(Boolean)
     select_as_cta_used = Column(Boolean, default=False)
 
@@ -915,19 +921,20 @@ def datasource(self):
     @datasource.getter
     @utils.memoized
     def get_datasource(self):
+        # pylint: disable=no-member
         ds = db.session.query(self.cls_model).filter_by(
             id=self.datasource_id).first()
         return ds
 
     @property
     def datasource_link(self):
-        return self.datasource.link
+        return self.datasource.link  # pylint: disable=no-member
 
     @property
     def roles_with_datasource(self):
         action_list = ''
-        pv = sm.find_permission_view_menu(
-            'datasource_access', self.datasource.perm)
+        perm = self.datasource.perm  # pylint: disable=no-member
+        pv = sm.find_permission_view_menu('datasource_access', perm)
         for r in pv.role:
             if r.name in self.ROLES_BLACKLIST:
                 continue
@@ -944,7 +951,7 @@ def roles_with_datasource(self):
     @property
     def user_roles(self):
         action_list = ''
-        for r in self.created_by.roles:
+        for r in self.created_by.roles:  # pylint: disable=no-member
             url = (
                 '/superset/approve?datasource_type={self.datasource_type}&'
                 'datasource_id={self.datasource_id}&'
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 6082ed1923a46..cd37d56b4c784 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -52,16 +52,16 @@ class AuditMixinNullable(AuditMixin):
         onupdate=datetime.now, nullable=True)
 
     @declared_attr
-    def created_by_fk(cls):  # noqa
+    def created_by_fk(self):  # noqa
         return sa.Column(
             sa.Integer, sa.ForeignKey('ab_user.id'),
-            default=cls.get_user_id, nullable=True)
+            default=self.get_user_id, nullable=True)
 
     @declared_attr
-    def changed_by_fk(cls):  # noqa
+    def changed_by_fk(self):  # noqa
         return sa.Column(
             sa.Integer, sa.ForeignKey('ab_user.id'),
-            default=cls.get_user_id, onupdate=cls.get_user_id, nullable=True)
+            default=self.get_user_id, onupdate=self.get_user_id, nullable=True)
 
     def _user_link(self, user):
         if not user:
diff --git a/superset/views/base.py b/superset/views/base.py
index 32ae3694f6d1c..7c15d69493e9a 100644
--- a/superset/views/base.py
+++ b/superset/views/base.py
@@ -103,12 +103,14 @@ def accessible_by_user(self, database, datasource_names, schema=None):
 
         role_ids = set([role.id for role in g.user.roles])
         # TODO: cache user_perms or user_datasources
+        PV = ab_models.PermissionView
+
+        pv_role = PV.role  # pylint: disable=no-member
         user_pvms = (
-            db.session.query(ab_models.PermissionView)
+            db.session.query(PV)
             .join(ab_models.Permission)
             .filter(ab_models.Permission.name == 'datasource_access')
-            .filter(ab_models.PermissionView.role.any(
-                ab_models.Role.id.in_(role_ids)))
+            .filter(pv_role.any(ab_models.Role.id.in_(role_ids)))
             .all()
         )
         user_perms = set([pvm.view_menu.name for pvm in user_pvms])
diff --git a/superset/views/core.py b/superset/views/core.py
index a1257d3396e38..25627c927d6f2 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -2017,7 +2017,7 @@ def sql_json(self):
             # Ignore the celery future object and the request may time out.
             try:
                 sql_lab.get_sql_results.delay(
-                    query_id, return_results=False,
+                    query_id=query_id, return_results=False,
                     store_results=not query.select_as_cta)
             except Exception as e:
                 logging.exception(e)
@@ -2047,7 +2047,9 @@ def sql_json(self):
                         "timeout. You may want to run your query as a "
                         "`CREATE TABLE AS` to prevent timeouts."
                     ).format(**locals())):
-                data = sql_lab.get_sql_results(query_id, return_results=True)
+                # pylint: disable=no-value-for-parameter
+                data = sql_lab.get_sql_results(
+                    query_id=query_id, return_results=True)
         except Exception as e:
             logging.exception(e)
             return json_error_response("{}".format(e))
diff --git a/superset/viz.py b/superset/viz.py
index ca341f43c26a1..ae440ef949a18 100755
--- a/superset/viz.py
+++ b/superset/viz.py
@@ -1064,7 +1064,7 @@ class DistributionBarViz(DistributionPieViz):
     is_timeseries = False
 
     def query_obj(self):
-        d = super(DistributionPieViz, self).query_obj()  # noqa
+        d = super(DistributionBarViz, self).query_obj()  # noqa
         fd = self.form_data
         gb = fd.get('groupby') or []
         cols = fd.get('columns') or []
diff --git a/tox.ini b/tox.ini
index d0a0c25464026..0827ba42d52fe 100644
--- a/tox.ini
+++ b/tox.ini
@@ -37,6 +37,13 @@ commands =
 [testenv:javascript]
 commands = {toxinidir}/superset/assets/js_build.sh
 
+[testenv:pylint]
+commands =
+  pip wheel -w {homedir}/.wheelhouse -f {homedir}/.wheelhouse .
+  pip install --find-links={homedir}/.wheelhouse --no-index .
+  pip install -r dev-reqs.txt
+  {toxinidir}/pylint-errors.sh
+
 [testenv:py27-mysql]
 basepython = python2.7
 setenv =