-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(ChartData): move ChartDataResult enums to common #17399
refactor(ChartData): move ChartDataResult enums to common #17399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM , simple move of 2 enums t
Codecov Report
@@ Coverage Diff @@
## master #17399 +/- ##
==========================================
+ Coverage 76.95% 77.03% +0.07%
==========================================
Files 1038 1039 +1
Lines 56026 56034 +8
Branches 7735 7735
==========================================
+ Hits 43113 43163 +50
+ Misses 12655 12613 -42
Partials 258 258
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are called ChartData*
, should we consider moving them to superset/charts
? And if not, should these actually be renamed to correspond more to the classes in superset/common
, so something like Query*
?
I agree with you but we should do it after we move all other components who use it to charts as well in case we will understand that is not conceptually possible, we should change the enums names |
I would prefer to start building towards the ultimate end state that we want to migrate towards rather than moving/refactoring the old code that we already know isn't optimal. For instance, if these class/type names don't make sense, I'd rather rename them now than move them, as moving doesn't really bring us any closer to the north star. And if we don't have a clear north star, let's rather spend some time discussing what that should be, so that we can gradually start introducing the new structures (along with comprehensive tests) that will eventually deprecate and totally remove the need for the old structures. |
so open a new naming discussion... in my opinion should be a general name for querying superset something that starts with SuperSet Query (SSQ) but note it can query a real datasource data but metadata and Supserset components as well |
Background
When we have worked on #16991 we wanted to test the new functionalities in concrete and accurate unittest.
All chartData flows and its components are too couple to superset so it is impossible to create unittests.
The flows are not testable and so many components do not meet the very important principle SRP and the code became so dirty
So I've started to refactor it (#17344 ) but many changes were added and it was hard to review so I decided to split those changes into small PRs so will be easier to follow
this is the First PR
The next PR is #17400
PR description
Moving ChartDataResultType and ChartDataResultFormat from utils to common.
think about it those are not utils and used by many places so it more sense to be in common
test plans
There are no logic change so new tests are not required