-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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.
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
Fix a bunch of files with pylint disabled #8743
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple suggestions
superset/jinja_context.py
Outdated
@@ -273,5 +272,5 @@ class HiveTemplateProcessor(PrestoTemplateProcessor): | |||
|
|||
|
|||
def get_template_processor(database, table=None, query=None, **kwargs): | |||
TP = template_processors.get(database.backend, BaseTemplateProcessor) | |||
return TP(database=database, table=table, query=query, **kwargs) | |||
tp = template_processors.get(database.backend, BaseTemplateProcessor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not name template_processor
?
superset/sql_lab.py
Outdated
database = query.database | ||
db_engine_spec = database.db_engine_spec | ||
parsed_query = ParsedQuery(sql_statement) | ||
sql = parsed_query.stripped() | ||
SQL_MAX_ROWS = app.config["SQL_MAX_ROW"] | ||
sql_max_rows = app.config["SQL_MAX_ROW"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be pulled out of the function to the top of the file? Then we keep the same var name but don't need to refetch from the app config each time
superset/sql_lab.py
Outdated
SQL_QUERY_MUTATOR = config["SQL_QUERY_MUTATOR"] | ||
if SQL_QUERY_MUTATOR: | ||
sql = SQL_QUERY_MUTATOR(sql, user_name, security_manager, database) | ||
sql_query_mutator = config["SQL_QUERY_MUTATOR"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here
Codecov Report
@@ Coverage Diff @@
## master #8743 +/- ##
=========================================
- Coverage 65.9% 65.9% -0.01%
=========================================
Files 483 483
Lines 24166 24166
Branches 2770 2770
=========================================
- Hits 15927 15926 -1
- Misses 8061 8062 +1
Partials 178 178
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming pylint and tests pass, lgtm.
one comment, but not required to address in this pass
superset/sql_lab.py
Outdated
import logging | ||
import uuid | ||
from contextlib import closing | ||
from datetime import datetime | ||
from sys import getsizeof | ||
from typing import Optional, Tuple, Union | ||
|
||
# pylint and isort disagree on the correct import order. | ||
# Let's have isort win. | ||
# pylint: disable=ungrouped-imports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add this to a base pylint rule disable? so that we don't need it on a bunch of files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m a little perplexed as to why pylint
is complaining here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Not a bad idea. Will update here before I forget.
CATEGORY
Choose one
SUMMARY
Re-enable pylint on a number of files with pylint globally disabled.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@john-bodley