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(ChartData): move ChartDataResult enums to common #17399

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

ofekisr
Copy link
Contributor

@ofekisr ofekisr commented Nov 11, 2021

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

Copy link
Member

@amitmiran137 amitmiran137 left a 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
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #17399 (f1fe952) into master (675ffaf) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Flag Coverage Δ
hive 81.49% <100.00%> (+<0.01%) ⬆️
mysql 81.92% <100.00%> (+<0.01%) ⬆️
postgres 81.93% <100.00%> (+<0.01%) ⬆️
presto 81.79% <100.00%> (?)
python 82.42% <100.00%> (+0.14%) ⬆️
sqlite 81.60% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/utils/core.py 90.01% <ø> (-0.14%) ⬇️
superset/charts/api.py 85.86% <100.00%> (+0.03%) ⬆️
superset/charts/post_processing.py 67.47% <100.00%> (+0.26%) ⬆️
superset/charts/schemas.py 100.00% <100.00%> (ø)
superset/common/chart_data.py 100.00% <100.00%> (ø)
superset/common/query_actions.py 92.85% <100.00%> (+0.10%) ⬆️
superset/common/query_context.py 90.95% <100.00%> (+0.04%) ⬆️
superset/common/query_object.py 92.89% <100.00%> (+0.04%) ⬆️
superset/reports/commands/execute.py 91.22% <100.00%> (ø)
superset/views/core.py 76.97% <100.00%> (+0.01%) ⬆️
... and 3 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 675ffaf...f1fe952. Read the comment docs.

Copy link
Member

@villebro villebro left a 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*?

@amitmiran137 amitmiran137 merged commit 45480f7 into apache:master Nov 11, 2021
@amitmiran137 amitmiran137 deleted the refactor/chart_data_flows branch November 11, 2021 09:41
@ofekisr
Copy link
Contributor Author

ofekisr commented Nov 11, 2021

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*?

@villebro

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

@villebro
Copy link
Member

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.

@ofekisr
Copy link
Contributor Author

ofekisr commented Nov 11, 2021

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

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 size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants