Create a new serialize_report fn
#554
Conversation
Swatinem
force-pushed
the
swatinem/report-serde
branch
2 times, most recently
from
dd3148d to
6ebee5a
Compare
CodSpeed Performance ReportMerging #554 will degrade performances by 21.68%Comparing Summary
Benchmarks breakdown
|
Swatinem
force-pushed
the
swatinem/report-serde
branch
from
6ebee5a to
996e82b
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
- Coverage 88.73% 88.71% -0.02%
==========================================
Files 461 461
Lines 13144 13159 +15
Branches 1510 1512 +2
==========================================
+ Hits 11663 11674 +11
- Misses 1164 1166 +2
- Partials 317 319 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
This merges the previous `to_database` and `to_archive` methods into one, which serializes a Report to the `report_json` and `chunks` pairs, and can optionally return a `totals` dict as well.
Swatinem
force-pushed
the
swatinem/report-serde
branch
from
02c0986 to
968f544
Compare
| @@ -26,6 +26,8 @@ | |||
| from shared.utils.sessions import Session, SessionType | |||
| from shared.utils.totals import agg_totals | |||
|
|
|||
| from .serde import _encode_chunk, serialize_report | |||
There was a problem hiding this comment.
haha serde. Rust background bubbling up
Or is that used in other contexts? I know it's from SerializeDeserialize, so it makes sense.
Don't love the relative import tho
| @@ -481,7 +483,9 @@ def __bool__(self): | |||
| @sentry_sdk.trace | |||
| def to_archive(self, with_header=True): | |||
| # TODO: confirm removing encoding here is fine | |||
| chunks = END_OF_CHUNK.join(_encode_chunk(chunk) for chunk in self._chunks) | |||
| chunks = END_OF_CHUNK.join( | |||
| _encode_chunk(chunk).decode() for chunk in self._chunks | |||
There was a problem hiding this comment.
Why make this function start with _?
That's typically a "private function" in Python, no?
There was a problem hiding this comment.
hum just to keep the same name as it was, I see I see
| @@ -506,6 +510,9 @@ def to_database(self): | |||
| ).decode(), | |||
| ) | |||
|
|
|||
| def serialize(self, with_totals=True) -> tuple[bytes, bytes, ReportTotals | None]: | |||
There was a problem hiding this comment.
I'd make it always return the totals, and you can just ignore them if not using...
Would that be a big performance impact? If that's the case I'd understand making it optional, but would make it more explicit
"only use totals if needed cause the function is considerably slower" or something
This merges the previous
to_databaseandto_archivemethods into one, which serializes a Report to thereport_jsonandchunkspairs, and can optionally return atotalsdict as well.