-
Notifications
You must be signed in to change notification settings - Fork 14k
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 chart_data_apis from ChartRestApi ChartDataRestApi #17400
refactor(ChartData): move chart_data_apis from ChartRestApi ChartDataRestApi #17400
Conversation
15c2826
to
9d26abb
Compare
Codecov Report
@@ Coverage Diff @@
## master #17400 +/- ##
==========================================
+ Coverage 76.95% 77.02% +0.06%
==========================================
Files 1039 1040 +1
Lines 56033 56074 +41
Branches 7735 7735
==========================================
+ Hits 43120 43189 +69
+ Misses 12655 12627 -28
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.
looks good, 3 endpoints we're moved from 1 module to another
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 second PR
The next PR is #17405
PR description
Create new view - ChartDataRestApi in new module but inherit ChartRestApi
then moves all chart data endpoints to that new view so the view will be focused only on chartData
Test plans
There are no logic changes so new tests are not required
but in the next PRs, I'll split the API tests into new separate modules and classes
Previous PRs