clean up status mixins #1004
clean up status mixins #1004
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1004 +/- ##
==========================================
- Coverage 97.79% 97.79% -0.01%
==========================================
Files 447 447
Lines 36154 36172 +18
==========================================
+ Hits 35357 35373 +16
- Misses 797 799 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
| @@ -6,25 +6,31 @@ | |||
|
|
|||
| log = logging.getLogger(__name__) | |||
|
|
|||
| STATE_SUCCESS = "success" | |||
There was a problem hiding this comment.
[nit] I'd use an Enum for this
|
|
||
| target_coverage: Decimal | None | ||
| totals = comparison.get_patch_totals() | ||
| def _get_target(self, comparison) -> Decimal | None: |
There was a problem hiding this comment.
[nit] typehint for comparison
| @@ -215,6 +233,7 @@ def _apply_adjust_base_behavior( | |||
| quantized_base_adjusted_coverage = base_adjusted_coverage.quantize( | |||
| Decimal("0.00000") | |||
| ) | |||
| ### shouldn't we use threshold here? | |||
There was a problem hiding this comment.
This is actually a fair question. I'd ask product if we want to do that.
I don't think we should... the idea of this is that the coverage needs to be better somehow for the check to pass (because it's already failing). The exact "how" is "looking at only adds, are they more covered than before?". Having the threshold can open this up too much.
There was a problem hiding this comment.
ooohhh, i see what you mean - I wasn't remembering that at this point in code they have already failed on the target + threshold base vs head measure.
I'll reach out to product because my thought here is - if they have a threshold, shouldn't we always be doing calculations with the threshold? And when we give them a message back, shouldn't we use/include threshold so that they don't sometimes see the number that represents the target, and other times the number that represents adjusted target with threshold?
| threshold = Decimal("0.0") | ||
| return threshold | ||
|
|
||
| def _get_target(self, base_report_totals) -> Decimal: |
There was a problem hiding this comment.
return typehint is incorrect, you are actually returning a tuple
There was a problem hiding this comment.
and there's no typehint on base_report_totals
There was a problem hiding this comment.
ty for catching! I changed to tuple at the last minute and forgot to update the typing!
nora-codecov
force-pushed
the
nora/1626
branch
from
cd8e1ae to
856c0e3
Compare
I'm working on this ticket and this ticket, which are changing the messaging coming out of this flow around
targetandthreshold.StatusPatchMixinandStatusProjectMixinare similar, but doing things differently, iehead_coverage + threshold >= target_coveragevshead_coverage >= (target_coverage - threshold)No behavior/logic changes, just cleanup to make them more uniform and easier to understand.