chore: make TS enums strictly PascalCase #26875

villebro · 2024-01-30T00:13:46Z

SUMMARY

TypeScript enum names and keys should always be PascalCase, which is the commonly accepted best practice (see the official docs: https://www.typescriptlang.org/docs/handbook/enums.html). This PR does the following:

Adds ESLint rules to enforce both PascalCase for the enum names and their respective keys.
Fix the whole FS codebase, including superset-ui packages/plugins. The enum values have been left in tact so as to not cause a breaking change.

Note, that I've made a few exceptions, where deviating from pascal case makes more sense, especially the locales and currencies, where it felt dangerous and less intuitive to move to pascal case.

TESTING INSTRUCTIONS

Pull the codebase, try to rename a TS enum to snake case, and notice how the linter complains.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
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 · 2024-01-30T22:16:58Z

Codecov Report

Attention: 184 lines in your changes are missing coverage. Please review.

Comparison is base (1f5ae74) 67.24% compared to head (e22ccab) 67.29%.
Report is 22 commits behind head on master.

Files	Patch %	Lines
...end/src/features/databases/DatabaseModal/index.tsx	30.76%	16 Missing and 2 partials ⚠️
...frontend/src/SqlLab/components/SqlEditor/index.tsx	8.33%	5 Missing and 6 partials ⚠️
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx	50.00%	7 Missing and 3 partials ⚠️
...SqlLab/components/KeyboardShortcutButton/index.tsx	0.00%	0 Missing and 8 partials ⚠️
...frontend/src/SqlLab/components/ResultSet/index.tsx	36.36%	2 Missing and 5 partials ⚠️
...tend/src/components/Datasource/CollectionTable.tsx	0.00%	3 Missing and 2 partials ⚠️
...end/src/components/Datasource/DatasourceEditor.jsx	0.00%	0 Missing and 5 partials ⚠️
.../controls/ContourControl/ContourPopoverControl.tsx	0.00%	5 Missing ⚠️
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx	0.00%	4 Missing ⚠️
...gin-chart-echarts/src/Timeseries/transformProps.ts	20.00%	0 Missing and 4 partials ⚠️
... and 66 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26875      +/-   ##
==========================================
+ Coverage   67.24%   67.29%   +0.04%     
==========================================
  Files        1895     1894       -1     
  Lines       74248    74246       -2     
  Branches     8257     8256       -1     
==========================================
+ Hits        49929    49963      +34     
+ Misses      22248    22208      -40     
- Partials     2071     2075       +4

Flag	Coverage Δ
javascript	`57.10% <49.58%> (+0.09%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

villebro · 2024-01-31T01:38:02Z

superset-frontend/src/explore/components/ChartPills.tsx

-enum CHART_STATUS_MAP {
-  failed = 'danger',
-  loading = 'warning',
-  success = 'success',
-}
+const CHART_STATUS_MAP = {
+  failed: 'danger',
+  loading: 'warning',
+  success: 'success',
+};


This was really not a proper enum or used as such, so I changed it to a regular object.

villebro · 2024-01-31T01:40:36Z

superset-frontend/src/components/Table/cell-renderers/NumericCell/index.tsx

@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/naming-convention */


I added this single override, as it felt sensible to leave the locales and currencies as-is

michael-s-molina

Thank you for the improvement @villebro. I left some first-pass comments.

michael-s-molina · 2024-01-31T14:34:18Z

superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts

+  ScheduledQueries = 'SCHEDULED_QUERIES',
+  ShareQueriesViaKvStore = 'SHARE_QUERIES_VIA_KV_STORE',
+  SqllabBackendPersistence = 'SQLLAB_BACKEND_PERSISTENCE',
+  SqlValidatorsByEngine = 'SQLLAB_BACKEND_PERSISTENCE',


Suggested change

SqlValidatorsByEngine = 'SQLLAB_BACKEND_PERSISTENCE',

SqlValidatorsByEngine = 'SQL_VALIDATORS_BY_ENGINE',

Eagle eyes! How did you even spot this?!

Lots of ☕ apparently! :)

☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕☕

superset-frontend/src/components/Table/cell-renderers/NumericCell/index.tsx

michael-s-molina

LGTM!

villebro · 2024-01-31T19:41:34Z

I'm gonna go ahead and merge this, as it conflicts extremely easily due to the number of touched files. Thanks for the reviews!

villebro · 2024-01-31T19:42:20Z

...but I'll leave it open until the end of the working day PST so anyone else can post commentts if they have time to review!

kgabryje

Love it, 100% recommend for bed time read if you have insomnia. LGTM!

rusackas · 2024-01-31T20:50:51Z

You know when you forget to blink for too long, then it hurts when you do?

I'm halfway done reviewing. My browser hates this PR, but I love that you've done it :D

rusackas · 2024-01-31T20:53:34Z

superset-frontend/src/SqlLab/reducers/sqlLab.js

@@ -332,15 +332,15 @@ export default function sqlLabReducer(state = {}, action) {
    },
    [actions.REQUEST_QUERY_RESULTS]() {
      return alterInObject(state, 'queries', action.query, {
-        state: QueryState.FETCHING,
+        state: QueryState.Fetching,


villebro · 2024-01-31T21:11:33Z

I'm halfway done reviewing. My browser hates this PR, but I love that you've done it :D

The fact that this PR is 2k LOC, 360+ files and almost crashes the browser shows just how bad this inconsistency problem had gotten over time. I look forward to doing the same for the backend codebase.. shudders

...nd/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx

superset-frontend/src/features/reports/types.ts

rusackas

LGTM!
...passes out

villebro requested review from michael-s-molina, geido and kgabryje as code owners January 30, 2024 00:13

villebro force-pushed the villebro/cleanup-enum branch from 63d3995 to 984b050 Compare January 30, 2024 00:20

chore: make TS enums strictly PascalCase

5021728

villebro force-pushed the villebro/cleanup-enum branch 2 times, most recently from a2ef0a3 to fed2486 Compare January 30, 2024 01:24

lint

4bacf57

villebro force-pushed the villebro/cleanup-enum branch from fed2486 to 4bacf57 Compare January 30, 2024 01:46

john-bodley added v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch risk:breaking-change Issues or PRs that will introduce breaking changes and removed v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch labels Jan 30, 2024

villebro added 2 commits January 30, 2024 10:38

Merge branch 'master' into villebro/cleanup-enum

2ed4f93

reinstate locale and currency enums

3a465ad

villebro force-pushed the villebro/cleanup-enum branch from 413ad45 to 3a465ad Compare January 30, 2024 21:19

fix renamed enum value

1cf235c

fix renamed values

ab62800

villebro force-pushed the villebro/cleanup-enum branch from 86030fc to ab62800 Compare January 31, 2024 00:59

fix lingering string based axistype comparison

139c8c0

pull-request-size bot added the size/XXL label Jan 31, 2024

villebro commented Jan 31, 2024

View reviewed changes

change enum to object

e22ccab

villebro force-pushed the villebro/cleanup-enum branch from dffe05e to e22ccab Compare January 31, 2024 01:58

michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jan 31, 2024

michael-s-molina requested changes Jan 31, 2024

View reviewed changes

michael-s-molina requested review from rusackas and justinpark January 31, 2024 15:52

revert en-US to LocaleCode.en_US

2863bba

Merge branch 'master' into villebro/cleanup-enum

d6b6ca4

michael-s-molina approved these changes Jan 31, 2024

View reviewed changes

kgabryje approved these changes Jan 31, 2024

View reviewed changes

rusackas reviewed Jan 31, 2024

View reviewed changes

...nd/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx Show resolved Hide resolved

kgabryje mentioned this pull request Jan 31, 2024

feat(Alerts and Reports): Modal redesign #26202

Merged

9 tasks

rusackas reviewed Jan 31, 2024

View reviewed changes

superset-frontend/src/features/reports/types.ts Outdated Show resolved Hide resolved

rusackas approved these changes Jan 31, 2024

View reviewed changes

villebro added 2 commits January 31, 2024 15:17

make PNG and CSV all caps

ee641eb

Merge branch 'master' into villebro/cleanup-enum

c41dbf3

villebro merged commit 19f8405 into apache:master Feb 1, 2024
31 checks passed

villebro deleted the villebro/cleanup-enum branch February 1, 2024 01:40

villebro mentioned this pull request Feb 1, 2024

chore: harmonize python enum naming conventions #26948

Draft

9 tasks

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

chore: make TS enums strictly PascalCase (apache#26875)

b8ec243

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

michael-s-molina mentioned this pull request Jun 14, 2024

fix: Custom SQL filter control #29260

Merged

9 tasks

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore: make TS enums strictly PascalCase (apache#26875)

020bffe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make TS enums strictly PascalCase #26875

chore: make TS enums strictly PascalCase #26875

villebro commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

villebro Jan 31, 2024

villebro Jan 31, 2024

michael-s-molina left a comment

michael-s-molina Jan 31, 2024

rusackas Jan 31, 2024

eschutho Jan 31, 2024

michael-s-molina Jan 31, 2024

michael-s-molina left a comment

villebro commented Jan 31, 2024

villebro commented Jan 31, 2024

kgabryje left a comment

rusackas commented Jan 31, 2024

rusackas Jan 31, 2024

villebro commented Jan 31, 2024 •

edited

Loading

rusackas left a comment •

edited

Loading

		@@ -1,3 +1,4 @@
		/* eslint-disable @typescript-eslint/naming-convention */

	SqlValidatorsByEngine = 'SQLLAB_BACKEND_PERSISTENCE',
	SqlValidatorsByEngine = 'SQL_VALIDATORS_BY_ENGINE',

chore: make TS enums strictly PascalCase #26875

chore: make TS enums strictly PascalCase #26875

Conversation

villebro commented Jan 30, 2024 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

villebro Jan 31, 2024

Choose a reason for hiding this comment

villebro Jan 31, 2024

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

michael-s-molina Jan 31, 2024

Choose a reason for hiding this comment

rusackas Jan 31, 2024

Choose a reason for hiding this comment

eschutho Jan 31, 2024

Choose a reason for hiding this comment

michael-s-molina Jan 31, 2024

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

villebro commented Jan 31, 2024

villebro commented Jan 31, 2024

kgabryje left a comment

Choose a reason for hiding this comment

rusackas commented Jan 31, 2024

rusackas Jan 31, 2024

Choose a reason for hiding this comment

villebro commented Jan 31, 2024 • edited Loading

rusackas left a comment • edited Loading

Choose a reason for hiding this comment

villebro commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

villebro commented Jan 31, 2024 •

edited

Loading

rusackas left a comment •

edited

Loading