Skip to content
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

refactor: convert controlUtils to TypeScript (1 of 2) #13401

Merged
merged 3 commits into from Mar 3, 2021

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Mar 1, 2021

SUMMARY

Convert getControlConfig and getSectionsToRender (renamed from sectionToRender) to TypeScript.

Will convert the remaining functions in src/explore/controlUtils in another PR.

Part of the larger effort of adding typing to the control panel.

Related: #13221 #13374

This PR is based on #13374 , will rebase to master once it is merged.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No functional change

TEST PLAN

  • CI should pass
  • Control panels should work exactly like before

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@ktmud ktmud requested a review from etr2460 March 1, 2021 23:46
@ktmud ktmud changed the base branch from master to ts-chart-reducer March 1, 2021 23:48
@junlincc junlincc added the explore:refactor Related to refactoring Explore label Mar 2, 2021
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Mar 2, 2021
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the typing looks reasonable, i'm assuming the rest was copy pasted


export const getControlConfig = function getControlConfig(
controlKey: string,
vizType: string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not have a type for vizType yet? Seems like it should exist...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it doesn't. I've thought about creating a VizType and actually did create a temporary type in an earlier iteration:

// TODO: replace this with string literals of actual supported viz types
type VizType = string;

But decided against it because it may not work well with dynamic plugins where viz types could be anything. If we do still find value in restricting viz types, we can do that while we convert MainPreset.js to TS.


// apply section overrides
Object.entries(sectionOverrides).forEach(([section, overrides]) => {
if (typeof overrides === 'object' && overrides.constructor === Object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need these safeguards still now that there's typing here? or is there a better way to do this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all controlPanel configs are TypeScript so I'd rather keep the same logic. We can do the cleanups after all plugins are migrated to TypeScript.

@ktmud ktmud changed the base branch from ts-chart-reducer to master March 3, 2021 00:31
@ktmud ktmud closed this Mar 3, 2021
@ktmud ktmud reopened this Mar 3, 2021
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #13401 (b90e535) into master (26e36ae) will decrease coverage by 3.93%.
The diff coverage is 89.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13401      +/-   ##
==========================================
- Coverage   77.44%   73.50%   -3.94%     
==========================================
  Files         903      604     -299     
  Lines       45662    21241   -24421     
  Branches     5506     5430      -76     
==========================================
- Hits        35361    15614   -19747     
+ Misses      10175     5502    -4673     
+ Partials      126      125       -1     
Flag Coverage Δ
cypress 57.99% <56.14%> (-0.01%) ⬇️
hive ?
javascript 63.42% <89.39%> (+0.01%) ⬆️
mysql ?
postgres ?
presto ?
python ?
sqlite ?

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

Impacted Files Coverage Δ
...perset-frontend/src/explore/components/Control.tsx 95.23% <ø> (ø)
.../src/explore/components/ControlPanelsContainer.tsx 97.27% <66.66%> (ø)
...nd/src/explore/controlUtils/getSectionsToRender.ts 84.37% <84.37%> (ø)
...ntend/src/explore/controlUtils/getControlConfig.ts 96.00% <96.00%> (ø)
...et-frontend/src/explore/controlPanels/sections.tsx 100.00% <100.00%> (ø)
...uperset-frontend/src/explore/controlUtils/index.js 100.00% <100.00%> (+4.58%) ⬆️
superset/sql_lab.py
superset/errors.py
superset/examples/world_bank.py
superset/utils/dict_import_export.py
... and 299 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 26e36ae...91d5669. Read the comment docs.

@ktmud ktmud merged commit dc1eb30 into apache:master Mar 3, 2021
@ktmud ktmud deleted the ts-control-utils branch March 3, 2021 22:51
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore:refactor Related to refactoring Explore size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants