chore(pylint): Bump Pylint to 2.9.6 #16146

john-bodley · 2021-08-09T16:07:57Z

SUMMARY

Bumping Pylint as recent versions address a number of false positives related to TYPE_CHECKING. See pylint-dev/pylint#3382 for more details. Sniffed out a couple of bugs in the process.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

Has associated issue: [SIP-46] Proposal for strict Pylint enforcement #9953
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-08-09T16:25:53Z

Codecov Report

Merging #16146 (aaa918e) into master (f946954) will decrease coverage by 0.05%.
The diff coverage is 52.77%.

@@            Coverage Diff             @@
##           master   #16146      +/-   ##
==========================================
- Coverage   76.73%   76.67%   -0.06%     
==========================================
  Files         997      997              
  Lines       53230    53069     -161     
  Branches     6771     6709      -62     
==========================================
- Hits        40844    40692     -152     
+ Misses      12156    12151       -5     
+ Partials      230      226       -4

Flag	Coverage Δ
hive	`?`
mysql	`81.56% <52.77%> (-0.02%)`	⬇️
postgres	`81.55% <52.77%> (-0.06%)`	⬇️
presto	`?`
python	`81.67% <52.77%> (-0.45%)`	⬇️
sqlite	`81.23% <52.77%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/dashboards/api.py	`93.03% <ø> (ø)`
superset/datasets/commands/importers/v1/utils.py	`58.97% <0.00%> (ø)`
superset/examples/flights.py	`18.75% <0.00%> (+1.10%)`	⬆️
superset/examples/helpers.py	`80.00% <0.00%> (ø)`
superset/models/core.py	`89.14% <0.00%> (-0.73%)`	⬇️
superset/models/schedules.py	`86.79% <ø> (ø)`
superset/models/tags.py	`78.94% <ø> (ø)`
superset/tasks/cache.py	`73.91% <0.00%> (ø)`
superset/tasks/schedules.py	`76.36% <ø> (ø)`
superset/utils/pandas_postprocessing.py	`84.80% <0.00%> (ø)`
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f946954...aaa918e. Read the comment docs.

john-bodley · 2021-08-12T04:06:44Z

superset/models/core.py

@@ -536,9 +536,10 @@ def get_all_table_names_in_schema(
            ]
        except Exception as ex:  # pylint: disable=broad-except
            logger.warning(ex)
+            return []


Kind of surprised Mypy didn't find this.

john-bodley · 2021-08-12T04:08:15Z

superset/models/tags.py

@@ -46,6 +46,7 @@ class TagTypes(enum.Enum):
    can find all their objects by querying for the tag `owner:alice`.
    """

+    # pylint: disable=invalid-name


I opted not the change these enums to uppercase as the string representation of the lowercase name is used throughout and I wanted to minimize the size of the PR.

john-bodley · 2021-08-12T04:09:06Z

superset/utils/core.py

@@ -1228,8 +1228,8 @@ def get_main_metric_name(metrics: Sequence[Metric]) -> Optional[str]:
 def ensure_path_exists(path: str) -> None:
    try:
        os.makedirs(path)
-    except OSError as exc:
-        if not (os.path.isdir(path) and exc.errno == errno.EEXIST):
+    except OSError as ex:


This fixes a potential bug as exc is defined globally.

john-bodley · 2021-08-12T04:09:34Z

superset/utils/pandas_postprocessing.py

@@ -593,7 +593,7 @@ def geohash_encode(
        )
        return _append_columns(df, encode_df, {"geohash": geohash})
    except ValueError:
-        QueryObjectValidationError(_("Invalid longitude/latitude"))
+        raise QueryObjectValidationError(_("Invalid longitude/latitude"))


Fixes a potential bug as the exception was never raised.

villebro · 2021-08-12T04:22:11Z

superset/cli.py

-        contents = {path.name: open(path).read() for path in files}
+            contents = {}
+            for path_ in files:
+                with open(path_) as file:
+                    contents[path_.name] = file.read()


This is slightly unrelated, but as you're bumping the indentation, I wonder if we should attempt importing at all if username is None? And if so (=we assume there may be use cases where an anonymous user can import), shouldn't this added indentation be restored to its original place?

villebro · 2021-08-12T04:24:17Z

superset/connectors/sqla/models.py

-            if not all([flt.get(s) for s in ["col", "op"]]):
+            if not all(flt.get(s) for s in ["col", "op"]):


If pylint caught this this is a nice improvement 👍

I know many people find Pylint somewhat verbose and cumbersome, but the linting rules and checks are far superior than other linters in my opinion.

villebro · 2021-08-12T04:27:35Z

superset/examples/flights.py

-        del pdf["YEAR"]
-        del pdf["MONTH"]
-        del pdf["DAY"]
+        del pdf["YEAR"]  # pylint: disable=unsupported-delete-operation
+        del pdf["MONTH"]  # pylint: disable=unsupported-delete-operation
+        del pdf["DAY"]  # pylint: disable=unsupported-delete-operation


Should we use drop() here instead?

villebro

LGTM once CI passes

john-bodley · 2021-08-12T14:20:03Z

superset/models/schedules.py

@@ -31,17 +31,20 @@


 class ScheduleType(str, enum.Enum):
+    # pylint: disable=invalid-name


I opted not the change these enums (and others) to uppercase as the string representation of the lowercase name is used in the database and I wanted to minimize the size of the PR.

…orts

…gies * upstream/master: (64 commits) check roles before fetching reports (#16260) chore: upgrade mypy and add type guards (#16227) fix: pivot columns with ints for name (#16259) chore(pylint): Bump Pylint to 2.9.6 (#16146) fix examples tab for dashboard (#16253) chore: bump superset-ui packages to 0.17.84 (#16251) chore: Shows the dataset description in the gallery dropdown (#16200) fix(Dashboard): Omnibar dropdown visibility and keyboard commands (#16168) chore: bump py version for integration test (#16213) fix: skip perms on query context update (#16250) refactor: external metadata fetch API (#16193) feat(dao): admin can remove self from object owners (#15149) fix(dashboard): cross filter chart highlight when filters badge icon clicked (#16233) fix: validate_parameters and query (#16241) fix: Remove Advanced Analytics tag for 2 charts (#16240) Revert "feat: Changing Dataset names (#16199)" (#16235) feat: Allow users to connect via legacy SQLA form (#16201) fix: remove encryption from db params (#16214) fix(Explore): Show the tooltip only when label does not fit the container in METRICS/FILTERS/GROUP BY/SORT BY of the DATA panel (#16060) Show/hide tooltips (#16192) ... # Conflicts: # superset/tasks/caching/cache_strategy.py

Co-authored-by: John Bodley <john.bodley@airbnb.com>

pull-request-size bot added the size/XS label Aug 9, 2021

john-bodley force-pushed the john-bodley--pylint-reenable-ungrouped-imports branch from ef95ea5 to 9049df5 Compare August 12, 2021 04:02

pull-request-size bot added size/L and removed size/XS labels Aug 12, 2021

john-bodley commented Aug 12, 2021

View reviewed changes

john-bodley force-pushed the john-bodley--pylint-reenable-ungrouped-imports branch 2 times, most recently from 9d9a1cf to eab5634 Compare August 12, 2021 04:22

villebro reviewed Aug 12, 2021

View reviewed changes

john-bodley force-pushed the john-bodley--pylint-reenable-ungrouped-imports branch 3 times, most recently from f1adab9 to c800822 Compare August 12, 2021 06:22

villebro approved these changes Aug 12, 2021

View reviewed changes

chore(pylint): Bump Pylint to 2.9.6

8c3d986

john-bodley force-pushed the john-bodley--pylint-reenable-ungrouped-imports branch from c800822 to 8c3d986 Compare August 12, 2021 06:26

john-bodley commented Aug 12, 2021

View reviewed changes

Merge branch 'master' into john-bodley--pylint-reenable-ungrouped-imp…

aaa918e

…orts

john-bodley merged commit 24b43be into apache:master Aug 13, 2021

john-bodley deleted the john-bodley--pylint-reenable-ungrouped-imports branch August 13, 2021 22:32

john-bodley mentioned this pull request Aug 13, 2021

chore(pylint): Reenable ungrouped-imports check #16256

Merged

9 tasks

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

chore(pylint): Bump Pylint to 2.9.6 (apache#16146)

215e358

Co-authored-by: John Bodley <john.bodley@airbnb.com>

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

chore(pylint): Bump Pylint to 2.9.6 (apache#16146)

7c0203d

Co-authored-by: John Bodley <john.bodley@airbnb.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(pylint): Bump Pylint to 2.9.6 #16146

chore(pylint): Bump Pylint to 2.9.6 #16146

john-bodley commented Aug 9, 2021 •

edited

codecov bot commented Aug 9, 2021 •

edited

john-bodley Aug 12, 2021

john-bodley Aug 12, 2021

john-bodley Aug 12, 2021

john-bodley Aug 12, 2021

villebro Aug 12, 2021

villebro Aug 12, 2021

john-bodley Aug 12, 2021

villebro Aug 12, 2021

villebro left a comment

john-bodley Aug 12, 2021

		if not all([flt.get(s) for s in ["col", "op"]]):
		if not all(flt.get(s) for s in ["col", "op"]):

		@@ -31,17 +31,20 @@


		class ScheduleType(str, enum.Enum):
		# pylint: disable=invalid-name

chore(pylint): Bump Pylint to 2.9.6 #16146

chore(pylint): Bump Pylint to 2.9.6 #16146

Conversation

john-bodley commented Aug 9, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 9, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Aug 9, 2021 •

edited

codecov bot commented Aug 9, 2021 •

edited