-
Notifications
You must be signed in to change notification settings - Fork 30
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
coverage over time return component ID and name #546
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #546 +/- ##
=======================================
- Coverage 95.79 95.78 -0.01
=======================================
Files 777 777
Lines 17253 17262 +9
=======================================
+ Hits 16526 16534 +8
- Misses 727 728 +1
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.
Additional details and impacted files@@ Coverage Diff @@
## main #546 +/- ##
==========================================
- Coverage 91.52% 91.52% -0.01%
==========================================
Files 602 602
Lines 16397 16406 +9
==========================================
+ Hits 15007 15015 +8
- Misses 1390 1391 +1
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! |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #546 +/- ##
==========================================
- Coverage 91.52% 91.52% -0.01%
==========================================
Files 602 602
Lines 16397 16406 +9
==========================================
+ Hits 15007 15015 +8
- Misses 1390 1391 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -5,6 +5,7 @@ type Component { | |||
} | |||
|
|||
type ComponentMeasurements { | |||
componentId: String! |
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.
Real change.
components = [ | ||
c_id for c_id in components if c_id.component_id in filters["components"] | ||
] | ||
components = [c for c in components if c.component_id in filters["components"]] | ||
|
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.
Real change. just rename var name
components_mapping = { | ||
component.component_id: component.name for component in components | ||
} | ||
|
||
queried_measurements = [ |
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.
Real change.
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.
Dictionary comprehension is pretty neat
@@ -453,6 +482,7 @@ def resolve_component_measurements( | |||
after=after, | |||
before=before, | |||
last_measurement=last_measurements_mapping.get(component_id), | |||
components_mapping=components_mapping, |
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.
Real change.
follow_imports = silent | ||
warn_no_return = False |
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.
Added this new rule to not check implicit return None
, not a fan of that.
@@ -80,42 +80,50 @@ def patch_totals(self) -> ReportTotals: | |||
class ComponentMeasurements: | |||
def __init__( | |||
self, |
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.
Mostly real changes on this file
|
||
@cached_property | ||
def name(self): | ||
def name(self) -> str: | ||
if self.components_mapping.get(self.component_id): |
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.
nit: this could be written as
return self.components_mapping.get(self.component_id, self.component_id) right?
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.
Not really because the shared.Components
object would always set the name
to be empty string if it is not defined (never None). So we would have dicts like:
components_mapping = {
"Id1": "",
"Id2": "Name1",
}
using the .get
would return the empty string as name now because it found the empty string name value for the ID, but ofc doing if "":
returns False
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
Have
ComponentMeasurements
type return both component ID and name, if no name is supplied in the YML file the field will default to the ID.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.