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 04:06:50 UTC

[GitHub] [incubator-superset] willbarrett opened a new pull request #10106: refactor: Re-enable pylint on 5 files

willbarrett opened a new pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106


   ### SUMMARY
   Re-enables pylint on 5 files with global disables. Some checks are fixed, others disabled on a per-instance basis.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   N/A
   
   ### TEST PLAN
   CI, PR review
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### Reviewers
   @john-bodley @craig-rueda @dpgaspar 


----------------------------------------------------------------
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

[GitHub] [incubator-superset] willbarrett commented on pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett commented on pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#issuecomment-647172513


   I think I've addressed comments - ready for another pass.


----------------------------------------------------------------
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

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
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

[GitHub] [incubator-superset] willbarrett merged pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett merged pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106


   


----------------------------------------------------------------
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

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#issuecomment-647057627


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=h1) Report
   > Merging [#10106](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a6390afb8972f315a0e5c87e618e21708e596f36&el=desc) will **increase** coverage by `4.90%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10106/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10106      +/-   ##
   ==========================================
   + Coverage   65.79%   70.70%   +4.90%     
   ==========================================
     Files         590      400     -190     
     Lines       31152    12909   -18243     
     Branches     3169     3174       +5     
   ==========================================
   - Hits        20497     9127   -11370     
   + Misses      10477     3665    -6812     
   + Partials      178      117      -61     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `53.06% <ø> (?)` | |
   | #javascript | `59.62% <ø> (+0.06%)` | :arrow_up: |
   | #python | `?` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/views/dashboard/views.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGFzaGJvYXJkL3ZpZXdzLnB5) | | |
   | [superset/charts/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | | |
   | [superset/views/sql\_lab.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3Mvc3FsX2xhYi5weQ==) | | |
   | [superset/views/database/forms.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvZm9ybXMucHk=) | | |
   | [superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | | |
   | [superset/connectors/sqla/\_\_init\_\_.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL19faW5pdF9fLnB5) | | |
   | [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | | |
   | [superset/datasets/commands/update.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | | |
   | [...migrations/versions/fb13d49b72f9\_better\_filters.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9mYjEzZDQ5YjcyZjlfYmV0dGVyX2ZpbHRlcnMucHk=) | | |
   | [superset/dashboards/filters.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJzLnB5) | | |
   | ... and [321 more](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=footer). Last update [a6390af...5fad2de](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r442895851



##########
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:
       I think that the comment applies to the block, meaning you can:
   ```python
   # pylint: disable=attribute-defined-outside-init
   dbcol.sum = dbcol.is_numeric
   dbcol.avg = dbcol.is_numeric
   ```
   https://docs.pylint.org/en/1.6.0/faq.html#is-it-possible-to-locally-disable-a-particular-message
   
   It'd be nice to have more scoping options in `pylint`, but block is a good place to start.




----------------------------------------------------------------
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

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r443054936



##########
File path: superset/connectors/sqla/views.py
##########
@@ -377,10 +384,14 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin):
         )
     }
 
-    def pre_add(self, table: "TableModelView") -> None:
+    def pre_add(  # pylint: disable=arguments-differ

Review comment:
       Oh nice, yeah, that resolves it. I thought it was complaining about the type annotation. Should have read closer.




----------------------------------------------------------------
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

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#issuecomment-647057627


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=h1) Report
   > Merging [#10106](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a6390afb8972f315a0e5c87e618e21708e596f36&el=desc) will **increase** coverage by `5.09%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10106/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10106      +/-   ##
   ==========================================
   + Coverage   65.79%   70.89%   +5.09%     
   ==========================================
     Files         590      400     -190     
     Lines       31152    12909   -18243     
     Branches     3169     3174       +5     
   ==========================================
   - Hits        20497     9152   -11345     
   + Misses      10477     3641    -6836     
   + Partials      178      116      -62     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `53.49% <ø> (?)` | |
   | #javascript | `59.62% <ø> (+0.06%)` | :arrow_up: |
   | #python | `?` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/views/chart/filters.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY2hhcnQvZmlsdGVycy5weQ==) | | |
   | [superset/models/sql\_lab.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NxbF9sYWIucHk=) | | |
   | [superset/commands/utils.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29tbWFuZHMvdXRpbHMucHk=) | | |
   | [superset/queries/filters.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcXVlcmllcy9maWx0ZXJzLnB5) | | |
   | [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | | |
   | [superset/common/tags.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29tbW9uL3RhZ3MucHk=) | | |
   | [superset/examples/deck.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvZGVjay5weQ==) | | |
   | [superset/db\_engine\_specs/kylin.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2t5bGluLnB5) | | |
   | [superset/views/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvYXBpLnB5) | | |
   | [superset/extensions.py](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXh0ZW5zaW9ucy5weQ==) | | |
   | ... and [323 more](https://codecov.io/gh/apache/incubator-superset/pull/10106/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=footer). Last update [a6390af...5fad2de](https://codecov.io/gh/apache/incubator-superset/pull/10106?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r442896400



##########
File path: superset/security/manager.py
##########
@@ -57,15 +56,15 @@
 logger = logging.getLogger(__name__)
 
 
-class SupersetSecurityListWidget(ListWidget):
+class SupersetSecurityListWidget(ListWidget):  # pylint: disable=too-few-public-methods

Review comment:
       should we deny-list this one `too-few-public-methods`?




----------------------------------------------------------------
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

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r442826337



##########
File path: superset/connectors/sqla/models.py
##########
@@ -461,7 +460,7 @@ def make_sqla_column_compatible(
         if db_engine_spec.allows_column_aliases:
             label = db_engine_spec.make_label_compatible(label_expected)
             sqla_col = sqla_col.label(label)
-        sqla_col._df_label_expected = label_expected
+        sqla_col._df_label_expected = label_expected  # pylint: disable=protected-access

Review comment:
       Should we make it public or accessible through a property?

##########
File path: superset/connectors/sqla/views.py
##########
@@ -377,10 +384,14 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin):
         )
     }
 
-    def pre_add(self, table: "TableModelView") -> None:
+    def pre_add(  # pylint: disable=arguments-differ

Review comment:
       `pre_add(self, item: "TableModelView"): -> None` will this satisfy pylint?

##########
File path: superset/result_set.py
##########
@@ -48,14 +47,14 @@ def dedup(l: List[str], suffix: str = "__", case_sensitive: bool = True) -> List
     """
     new_l: List[str] = []
     seen: Dict[str, int] = {}
-    for s in l:
-        s_fixed_case = s if case_sensitive else s.lower()
+    for string in l:

Review comment:
       nit: We already know it's a string, I propose calling `item` instead of `string`

##########
File path: superset/security/manager.py
##########
@@ -273,7 +276,9 @@ def can_access_datasource(self, datasource: "BaseDatasource") -> bool:
             "datasource_access", datasource.perm or ""
         )
 
-    def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
+    def get_datasource_access_error_msg(  # pylint: disable=no-self-use

Review comment:
       ```
   @staticmethod
   def get_datasource_access_error_msg(
       self, datasource: "BaseDatasource"
       ) -> str:
   ...
   ```
   

##########
File path: superset/security/manager.py
##########
@@ -284,7 +289,9 @@ def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
         return f"""This endpoint requires the datasource {datasource.name}, database or
             `all_datasource_access` permission"""
 
-    def get_datasource_access_link(self, datasource: "BaseDatasource") -> Optional[str]:
+    def get_datasource_access_link(  # pylint: disable=unused-argument,no-self-use

Review comment:
       `@staticmethod`




----------------------------------------------------------------
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

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r442620065



##########
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:
       OK, what's the deal with these two attributes? Is this cruft? Can I kill it? does anyone know?




----------------------------------------------------------------
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

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r443052433



##########
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:
       You know, I thought about that, even tried it, and I'm pretty sure that'll break the function:
   
   ```python
   >>> from sqlalchemy.sql import column
   >>> column("test") is not None
   True
   >>> column("test") is None
   False
   >>> column("test") == None
   <sqlalchemy.sql.elements.BinaryExpression object at 0x108445e48>
   >>> column("test") != None
   <sqlalchemy.sql.elements.BinaryExpression object at 0x108450668>
   ```
   I believe this is building up a `where` clause, not returning booleans.




----------------------------------------------------------------
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

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r443056082



##########
File path: superset/security/manager.py
##########
@@ -57,15 +56,15 @@
 logger = logging.getLogger(__name__)
 
 
-class SupersetSecurityListWidget(ListWidget):
+class SupersetSecurityListWidget(ListWidget):  # pylint: disable=too-few-public-methods

Review comment:
       I'll disable it for the file.




----------------------------------------------------------------
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

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10106: refactor: Re-enable pylint on 5 files

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10106:
URL: https://github.com/apache/incubator-superset/pull/10106#discussion_r443050966



##########
File path: superset/connectors/sqla/models.py
##########
@@ -461,7 +460,7 @@ def make_sqla_column_compatible(
         if db_engine_spec.allows_column_aliases:
             label = db_engine_spec.make_label_compatible(label_expected)
             sqla_col = sqla_col.label(label)
-        sqla_col._df_label_expected = label_expected
+        sqla_col._df_label_expected = label_expected  # pylint: disable=protected-access

Review comment:
       Unfortunately the type on this is `Column`, which comes direct from SQLAlchemy. I'd prefer not to monkeypatch their internals.




----------------------------------------------------------------
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