Fix lint in superset/db_engine_spec
#8338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
❤️ 👍 |
mistercrunch
reviewed
Oct 2, 2019
|
Yes, it appears so - I think I just need to update a couple tests. |
added 11 commits
October 2, 2019 17:09
bad method signature
different method naming
willbarrett
force-pushed
the
wbarrett/db-engine-spec-lint
branch
from
2f343e9 to
79f4965
Compare
|
OK, CI is green - sorry for the many commits. Let me know if you'd like me to squash before merge. |
|
We force "Squash and merge" on this repo, so every PR gets auto-squashed and we have a 1-to-1 between commits in master and PRs. |
mistercrunch
reviewed
Oct 4, 2019
| @@ -841,47 +843,6 @@ def _process_array_data( | |||
| array_value[array_child] = "" | |||
| return all_array_data | |||
|
|
|||
| @classmethod | |||
| def _consolidate_array_data_into_data( | |||
There was a problem hiding this comment.
unused? @betodealmeida is this used in a cherry or something?
There was a problem hiding this comment.
Yes, #8233 simplified the logic and left some methods that are no longer used.
serenajiang
pushed a commit
to airbnb/superset-fork
that referenced
this pull request
Oct 9, 2019
* Enable lint checking for files in db_engine_spec that have few to no lint issues * Enable lint and fix issue in db_engine_spec/mysql.py * Enable pylint and fix lint for db_engine_spec/pinot.py * Enable lint and fix issues for db_engine_specs/hive.py * Enable lint and fix for db_engine_spec/presto.py * Re-enable lint on base.py, fix/disable specific failures, including one bad method signature * Make flake8 happy after a number of pylint fixes * Update db_engine_spec_test test cases related to Presto to support different method naming * automated reformatting * One more pylint disable for druid.py * Find the magic invocation that makes all the lint tools happy
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CATEGORY
Choose one
SUMMARY
The
db_engine_specfolder was largely un-linted by pylint, due to the presence of the# pylint: disable=C,R,Wcomment in most of the files. This method of skipping lint checks will hide all pylint failures introduced into the files. This PR removes those lines, fixes a number of pylint errors, and uses a more targeted disable approach for issues that would require larger refactors to resolve. Using this approach for disabling lint should highlight new problems introduced in future PRs and creates a specific list of items to clean up down the road.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
Explanation: there was a function in
presto.pythat I believe was un-referenced:_consolidate_array_data_into_data.REVIEWERS