-
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(ChartDataCommand): separate loading query_context form cache into different module #17405
refactor(ChartDataCommand): separate loading query_context form cache into different module #17405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17405 +/- ##
==========================================
+ Coverage 76.94% 76.96% +0.01%
==========================================
Files 1040 1041 +1
Lines 56074 56060 -14
Branches 7735 7735
==========================================
Hits 43146 43146
+ Misses 12670 12656 -14
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.
|
… into different module
1cfcd9b
to
c1931b5
Compare
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!
… into different module (#17405)
Background
When we have worked on #16991 we wanted to test the new functionalities in concrete and accurate unittest.
All The 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 third PR in a sequence of PRs to meet these
The next PR is #17407
PR description
The CharaDataCommand responsible is to retrieve data given query context parameters.
What it needs is query_context injected to him so it can fulfill its task. Loading the query_contxt form from the cache is another responsible, thus should be in another module to meet the SRP principle.
The module that is responsible for creating the DataChartCommand and injecting what the command needs, should charge for calling the cache loader.
more small changes:
add "_" prefix to methods name that supposed to be private and remove unused code
Test plans
There are no logic changes, the way the cache is loading is the same as was in the command so new tests are not required
Previous PRs