Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(control-utils): add shared controls + dependencies, convert to typescript #459

Merged
merged 2 commits into from
May 13, 2020

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented May 8, 2020

🏆 Enhancements

Brought in the remaining shared controls from incubator-superset. This is a prerequisite for moving many of the controlPanels into the chart plugins.

When typing the shared-controls module, I did my best but had to use unknown in several places. I propose merging the types as-is, and putting together a set of properly fleshed-out types for the controls system separately, since that will be very useful for plugin writers/maintainers. I don't feel well-equipped to do that myself.

There are many subtle changes to these files due to differences in typescript/linter requirements between projects. Looking at the diff between these files and their originals in incubator-superset might be the best way to review.

I know this brings test coverage down, but I don't have the bandwidth to write tests for everything in shared-controls.tsx. I don't think adding tests would actually provide any real quality improvements anyway, since those functions are so closely tied to external behavior and have almost no behavior of their own.

@vercel
Copy link

vercel bot commented May 8, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/75ze770q4
✅ Preview: https://superset-ui-git-fork-suddjian-controls-dependencies.superset.now.sh

@suddjian
Copy link
Member Author

suddjian commented May 8, 2020

This should be the last piece necessary to add to control-utils in order to allow moving the rest of the controlPanels over.

(except the DeckGL charts, they have some other dependency stuff going on that will need to be handled separately)

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #459 into master will increase coverage by 0.35%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
+ Coverage   22.38%   22.74%   +0.35%     
==========================================
  Files         270      275       +5     
  Lines        6562     6670     +108     
  Branches      608      644      +36     
==========================================
+ Hits         1469     1517      +48     
- Misses       5056     5113      +57     
- Partials       37       40       +3     
Impacted Files Coverage Δ
.../superset-ui-control-utils/src/shared-controls.tsx 19.40% <19.40%> (ø)
.../superset-ui-control-utils/src/ColumnTypeLabel.tsx 73.68% <73.68%> (ø)
...ges/superset-ui-control-utils/src/ColumnOption.tsx 91.66% <91.66%> (ø)
...ackages/superset-ui-control-utils/src/constants.ts 100.00% <100.00%> (ø)
packages/superset-ui-control-utils/src/index.ts 100.00% <100.00%> (ø)
...ckages/superset-ui-control-utils/src/mainMetric.ts 100.00% <100.00%> (ø)
... and 1 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 b569c34...a415cb7. Read the comment docs.

@ktmud
Copy link
Contributor

ktmud commented May 8, 2020

I'm making changes to the code coverage check: #468

@kristw
Copy link
Contributor

kristw commented May 12, 2020

Please rebase from master and that should resolve the coverage

@suddjian
Copy link
Member Author

Rebased and should be ready for merge pending CI

@suddjian
Copy link
Member Author

This is still failing coverage after rebasing. I'd like to just merge it since tests are passing and we've agreed to forego coverage for these files.

@ktmud
Copy link
Contributor

ktmud commented May 13, 2020

Damn, the coverage rule still does not work, as .ts now becomes partial match that also matches .tsx.

@ktmud ktmud merged commit befe0c7 into apache-superset:master May 13, 2020
@ktmud
Copy link
Contributor

ktmud commented May 13, 2020

Merged and created a PR to fix the coverage in the meantime: #482

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants