Unify ReportBuilderSession usage within LanguageProcessors
#684
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #684 +/- ##
========================================
Coverage 98.09% 98.09%
========================================
Files 433 434 +1
Lines 36752 36548 -204
========================================
- Hits 36051 35852 -199
+ Misses 701 696 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #684 +/- ##
========================================
Coverage 98.09% 98.09%
========================================
Files 433 434 +1
Lines 36752 36548 -204
========================================
- Hits 36051 35852 -199
+ Misses 701 696 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #684 +/- ##
========================================
Coverage 98.09% 98.09%
========================================
Files 433 434 +1
Lines 36752 36548 -204
========================================
- Hits 36051 35852 -199
+ Misses 701 696 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #684 +/- ##
========================================
Coverage 98.13% 98.13%
========================================
Files 475 475
Lines 38107 37903 -204
========================================
- Hits 37396 37198 -198
+ Misses 711 705 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
services/report/report_builder.py
Outdated
| def create_coverage_line( | ||
| self, | ||
| coverage, | ||
| coverage: int, |
There was a problem hiding this comment.
this isn't always an int :( sometimes it is a fraction represented as a string (example: https://github.com/codecov/worker/blob/50142115/services/report/languages/bullseye.py#L73)
according to the types in shared, it's actually a Decimal, but i have not seen that be true https://github.com/codecov/shared/blob/36c00afb1ad34185c795227816ceb9eaa252645d/shared/reports/types.py#L135
in Node it can apparently be a Fraction https://github.com/codecov/worker/blob/50142115/services/report/languages/node.py#L308. except that's just in the partials field, which according to the types in shared is Sequence[int] (which is wrong, i've only ever seen [(start_col, end_col, coverage)] (we drop the start/end lines))
as an aside: my local https://livegrep.com/search/linux instance is really helpful for codebase exploration like this. i searched cov(erage)? = path:services/report/langu and (Fraction|Decimal) repo:worker|shared -path:migrations|models to find those code pointers. my setup indexes worker, shared, api, codecov-rs, and more, is much faster/more responsive than github search, and is written up at https://l.codecov.dev/kPqYLC (open on VPN) if you want to run your own
There was a problem hiding this comment.
I changed this to int | str now. The type in shared being wider than this is fine.
Also in general I don’t really trust the type annotations at all (yet), so there might be some followups to do.
| filename = method.attrib["filename"] | ||
| if filename not in files: | ||
| _file = report_builder_session.create_coverage_file(filename) | ||
| files[filename] = _file | ||
| _file = files[filename] | ||
| if _file is None: |
There was a problem hiding this comment.
to be thorough: it's okay that the key in files is the unfixed path, right? it doesn't look like we try to use the fixed filename from the coverage file as a key ever, so i think so
There was a problem hiding this comment.
yes I think thats fine, I even think it might be good for perf as the path fixing can be slow sometimes. Some other language processors seem to cache that stuff, but usage is definitely not consistent.
| report_builder_session.ignored_lines, | ||
| ) | ||
|
|
||
| files: dict[str, ReportFile] = {} |
There was a problem hiding this comment.
probably fine, but this has changed scope and will not clear files between "modules"
wish i understood this format to know whether that's important
services/report/languages/xcode.py
Outdated
| @@ -70,12 +67,6 @@ def get_partials_in_line(line): | |||
|
|
|||
|
|
|||
| def from_txt(content: bytes, report_builder_session: ReportBuilderSession) -> Report: | |||
There was a problem hiding this comment.
one day codecov-rs will have samples and documentation for this so reviewing it isn't so difficult :P
Switches to using `create_coverage_line` instead of creating `ReportLine`s directly. And switches to using `.append()` instead of relying on an assignment operator. This also includes some simplifications like moving `read_yaml_field` out of loops, etc.
Swatinem
force-pushed
the
swatinem/report-builder
branch
from
d798aa0 to
b82aab4
Compare
This includes various refactorings:
Switches to using
create_coverage_lineinstead of creatingReportLines directly.And switches to using
.append()instead of relying on an assignment operator.This also includes some simplifications like moving
read_yaml_fieldout of loops, etc.Switches all
LanguageProcessors to usingReportBuilderSession, and pass such aReportBuilderSessionto theprocessmethod.Simplifies the signature of
create_coverage_lineand its usage, and introduces a newcreate_coverage_filemethod as well which incorporates thePathFixerapplication and the ignored lines.fixes codecov/engineering-team#2392