-
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
Consolidate implementation of calculate_diff
#528
Conversation
d008aaf
to
e454565
Compare
CodSpeed Performance ReportMerging #528 will create unknown performance changesComparing Summary
|
e454565
to
ec03cd7
Compare
This cleans up the duplicated implementations of `Filtered/Report.calculate_diff` and moves it to a new well typed file.
ec03cd7
to
f664924
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. Thanks for adding more types to this part of the code 🥳
As you mentioned no speed improvement, but in the comparison the stddev for the Report
class was greatly reduced... that might be a win? Having a more bounded code I guess. The speed report doesn't really report quantiles for us to know, though.
I wonder if using @dataclass
instead of TypedDict
could improve speed. But my guess is that it would be worse (cause you need to transform from dict into a class instance).
|
||
class DiffSegment(TypedDict): | ||
lines: list[str] | ||
header: tuple[int, int, int, int] |
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.
A comment here like # (base_start_line, base_length, head_start_line, head_length)
would be useful for people that don't know what the numbers represent
Or breaking up the header
further, but might be overkill
shared/reports/diff.py
Outdated
|
||
|
||
class DiffFile(TypedDict): | ||
type: str |
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.
do we know what the possible types are?
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.
also lgtm:
to answer one of @giovanni-guidini's questions from above:
Line 142 in 3bcf3c9
def diff_to_json(self, diff): |
I got confused because i was looking at the response schemas from the Github API and I couldn't find "type" or "segments"
This cleans up the duplicated implementations of
Filtered/Report.calculate_diff
and moves it to a new well typed file.I’m a bit surprised that this does not yield any speedup in the diff calculation benchmarks (compared to #525).
I think the difference between the base and
FilteredReport
is based on the actual filtering and line-mapping code that happens, instead of the different and less optimized version that was previously there.