-
Notifications
You must be signed in to change notification settings - Fork 8
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
Merge EditableReportFile
into ReportFile
#548
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
==========================================
- Coverage 88.51% 88.50% -0.02%
==========================================
Files 451 451
Lines 13127 13107 -20
Branches 1529 1528 -1
==========================================
- Hits 11620 11600 -20
Misses 1184 1184
Partials 323 323
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
❌ 16 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 16 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
CodSpeed Performance ReportMerging #548 will create unknown performance changesComparing Summary
|
@@ -424,6 +454,38 @@ def line_without_labels( | |||
sessions=new_sessions, | |||
) | |||
|
|||
def delete_labels( |
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.
Is it worth documenting somehow that these functionalities used to belong to an EditableReport and that should be used only in particular contexts? I guess we're not making that distinction any more since we're merging Report related classes, so I guess it's fair game to use any property/methods since they're moved to the ReportFile
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.
Yes, it should be fair game to use any method now.
In reality, all the mutating methods were already part of the previous interface anyway.
The main purpose of the `EditableReportFile` was to offer methods to delete labels/sessions, as well as to properly maintain uptodate versions of `present_sessions` and `totals` when doing mutations on the file. All the methods were now moved to `ReportFile`, which has also gained new functionality related to making sure that caches are cleared when necessary, and re-computed on-demand, so that they are always correct. --- I plan to do a similar thing for `EditableReport` as well in a followup, as it has a similar purpose in relation to maintaining uptodate `totals`.
676f04d
to
4f96ce2
Compare
The main purpose of the
EditableReportFile
was to offer methods to delete labels/sessions, as well as to properly maintain uptodate versions ofpresent_sessions
andtotals
when doing mutations on the file.All the methods were now moved to
ReportFile
, which has also gained new functionality related to making sure that caches are cleared when necessary, and re-computed on-demand, so that they are always correct.I plan to do a similar thing for
EditableReport
as well in a followup, as it has a similar purpose in relation to maintaining uptodatetotals
.This is part of codecov/engineering-team#3381