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

Implement ChartClient #44

Closed
wants to merge 9 commits into from
Closed

Implement ChartClient #44

wants to merge 9 commits into from

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Nov 30, 2018

🏆 Enhancements

Create ChartClient to facilitate calls to various APIs necessary for embeddable charts.

Refer to this design doc

Working on unit tests.

@kristw kristw requested a review from a team as a code owner November 30, 2018 19:52
@kristw kristw added the WIP Work in progress label Nov 30, 2018
@codecov
Copy link

codecov bot commented Nov 30, 2018

Codecov Report

Merging #44 into master will decrease coverage by 12.44%.
The diff coverage is 17.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #44       +/-   ##
==========================================
- Coverage   99.35%   86.9%   -12.45%     
==========================================
  Files          64      65        +1     
  Lines         617     672       +55     
  Branches       51      73       +22     
==========================================
- Hits          613     584       -29     
- Misses          2      78       +76     
- Partials        2      10        +8
Impacted Files Coverage Δ
.../src/registries/ChartComponentRegistrySingleton.ts 100% <ø> (ø)
...t/src/registries/ChartMetadataRegistrySingleton.ts 100% <ø> (ø)
...registries/ChartTransformPropsRegistrySingleton.ts 100% <ø> (ø)
packages/superset-ui-chart/src/query/FormData.ts 100% <ø> (ø) ⬆️
...src/registries/ChartBuildQueryRegistrySingleton.ts 100% <ø> (ø)
...kages/superset-ui-connection/src/SupersetClient.ts 33.69% <100%> (-63.18%) ⬇️
...kages/superset-ui-chart/src/clients/ChartClient.ts 11.11% <11.11%> (ø)
...superset-ui-connection/test/utils/throwIfCalled.ts 0% <0%> (-100%) ⬇️

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 0ab21ac...3a6d160. Read the comment docs.

Copy link
Contributor

@xtinec xtinec left a comment

Choose a reason for hiding this comment

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

Might be a good idea to type this change with TS.

@williaster
Copy link
Contributor

yeah, I guess we should align on

  1. should all new code in @superset-ui be TS?
  2. is this a requirement for all code that will be moved from Superset to Superset-UI?

I would generally support 1), but 2) could be a lot of work (similar to trying to get 100% test coverage on all the vis code).

@kristw
Copy link
Contributor Author

kristw commented Dec 5, 2018

I agree with end goal of having all @superset-ui being TS, but to maintain balance of making progress and code quality. I propose we let the js in now (with 100% test) and create accompany issue to convert the piece of code into TS, which can be addressed later.

@kristw kristw added the #enhancement New feature or request label Dec 6, 2018
@kristw kristw mentioned this pull request Dec 11, 2018
@kristw kristw closed this Dec 11, 2018
@kristw kristw deleted the kristw--chart-client branch February 5, 2019 22:30
kristw pushed a commit that referenced this pull request Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants