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

feat: allow developer to get supersetclient instance #552

Merged
merged 1 commit into from
May 30, 2020
Merged

Conversation

kristw
Copy link
Contributor

@kristw kristw commented May 30, 2020

🏆 Enhancements

In the existing implementation, SupersetClient.getInstance() will always throw error unless you also pass a SupersetClient instance as an argument. Then it will just return what you pass in as an argument back.

I believe the original intention was to force developer to call SupersetClient.configure() before calling .getInstance() but even after calling .configure(). The .getInstance() itself is still not useful by itself. Calling it without argument, hoping to obtained the configured SupersetClient results in an Error. (I want to get the host name from the already configured singleton) and if I already have the SupersetClient instance, I would not bother calling this function.

This PR modifies the code to preserve the original intention, but allow .getInstance() to return the configured SupersetClient if it already exists. (The only way for it to exist is somebody must have called .configure() before that.)

Technically this is breaking but I have checked the relevant repositories and none call .getInstance() (not surprising though given its limited usefulness, if any)

@kristw kristw requested a review from a team as a code owner May 30, 2020 00:44
@vercel
Copy link

vercel bot commented May 30, 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/cwhghpcpv
✅ Preview: https://superset-ui-git-kristw-client.superset.now.sh

@codecov
Copy link

codecov bot commented May 30, 2020

Codecov Report

Merging #552 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #552   +/-   ##
=======================================
  Coverage   23.13%   23.13%           
=======================================
  Files         290      290           
  Lines        6783     6783           
  Branches      676      676           
=======================================
  Hits         1569     1569           
  Misses       5176     5176           
  Partials       38       38           
Impacted Files Coverage Δ
...kages/superset-ui-connection/src/SupersetClient.ts 100.00% <100.00%> (ø)

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 bbc42c2...6367441. Read the comment docs.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

ah interesting. I think you're right about the original intention, in any case this makes more sense to me 👍

@kristw kristw merged commit c075070 into master May 30, 2020
@delete-merged-branch delete-merged-branch bot deleted the kristw--client branch May 30, 2020 00:57
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.

2 participants