chore: harmonize python enum naming conventions #26948
Conversation
There was a problem hiding this comment.
@villebro it would be great if pylint could prevent any future regressions. Per pylint-dev/pylint#3896 it seems like this functionality exists, so I'm a little perplexed as to why it's not working.
|
Thanks for the pointing me in the right direction @john-bodley! I agree, this seems to be covered by vanilla |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26948 +/- ##
==========================================
+ Coverage 60.48% 65.35% +4.86%
==========================================
Files 1931 537 -1394
Lines 76236 39066 -37170
Branches 8568 0 -8568
==========================================
- Hits 46114 25532 -20582
+ Misses 28017 13534 -14483
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| object_id=1, object_type=ObjectType.dashboard.name, tag_id=tag.id | ||
| object_id=1, object_type=ObjectType.DASHBOARD, tag_id=tag.id |
There was a problem hiding this comment.
@john-bodley the fact that my IDE was able to identify these type errors indicates that our /tests/ directory is in need of some serious linting love. I think the time has come to enable full linting on our Python test suites. WDYT? I can't remember how many bycatch unused imports I've removed over the last few years. I can probably start by cleaning up the unit test suite, as that seems to be the simplest place to start..
There was a problem hiding this comment.
@villebro I’ve been making some rather XXL refactors recently and wished that we had mypy and pylint checks for the entirety of the Python code base including tests and migrations.
There was a problem hiding this comment.
I've found mypy can be very temperamental, but mostly that's in terms of checking far beyond the scope of what it's being asked to in the first place. It does do a good job of catching problems when it's not a false positive though. pylint is much less temperamental so at least having that seems a no-brainer.
villebro
force-pushed
the
villebro/py-enum
branch
from
befa87a to
0c9978d
Compare
|
michael-s-molina
added
the
v4.0
|
This turned ugly, just as I suspected 🤦 Link to a Slack thread that I opened to get some feedback: https://apache-superset.slack.com/archives/CCKHMGRRB/p1706829726367459?thread_ts=1706828839.332729&cid=CCKHMGRRB |
michael-s-molina
added
v4.0
michael-s-molina
added
v5.0
|
Converting this to a draft while it's still in need of a rebase. Hopefully it gets more attention, but if not, it might be closed in time. |
|
I'll work on 5.0 tasks this week, and I'll see if this is still relevant. |
|
Incidentally not sure if others have used it but I've heard good things about |
|
Oops, guess I'm behind the times, I should have checked the project :D |
|
(I see the PR also mentions |
villebro
force-pushed
the
villebro/py-enum
branch
from
0c9978d to
baa2724
Compare
villebro
force-pushed
the
villebro/py-enum
branch
from
a50fdf6 to
ce20c97
Compare
villebro
force-pushed
the
villebro/py-enum
branch
from
ce20c97 to
a62349b
Compare
| if datasource_id is not None: | ||
| if datasource_id is not None and datasource_type is not None: |
There was a problem hiding this comment.
bycatch, a more recent linter would have likely started complaining about this sooner or later.
villebro
force-pushed
the
villebro/py-enum
branch
from
353b236 to
bda81d2
Compare
| summary: Get all objects associated with a tag | ||
| parameters: | ||
| - in: path | ||
| schema: | ||
| type: integer | ||
| name: tag_id | ||
| summary: Bulk create tags and tagged objects | ||
| requestBody: | ||
| description: Tag schema | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| tags: | ||
| description: list of tag names to add to object | ||
| type: array | ||
| items: | ||
| type: string | ||
| objects_to_tag: | ||
| description: list of object names to add to object | ||
| type: array | ||
| items: | ||
| type: array | ||
| $ref: '#/components/schemas/TagPostBulkSchema' |
There was a problem hiding this comment.
The tags API and related schemas are in pretty rough shape. I would almost recommend doing a full refactor of these in some future version.
SUMMARY
This is a continuation of #26875, and refactors all Python enums to comply with the commonly used naming conventions of PascalCase for the class name, and UPPER_CASE for the names: https://docs.python.org/3/library/enum.html This exercise was quite a bit less painful than on the frontend. 😸
Unfortunately I'm not aware of anything that would come close to ESLint for Python, so for now we'll just have to try to make sure everyone adheres to these conventions during code review. I believe something similar might be possible with pylint, but I wasn't able to scope it to only apply to enums, so I'll just leave it be. But if someone knows how this can be done, or can point me in the right direction, please do let me know.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION