-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix notifier without any layout #826
Conversation
This accounts for the case when `settings` does not have any `"layout"`. Also cleans up the code a little bit, using newer type annotations, and removing statsd timers.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01%
==========================================
Files 442 441 -1
Lines 36163 36048 -115
==========================================
- Hits 35448 35333 -115
Misses 715 715
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 #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01%
==========================================
Files 442 441 -1
Lines 36163 36048 -115
==========================================
- Hits 35448 35333 -115
Misses 715 715
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 #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01%
==========================================
Files 442 441 -1
Lines 36163 36048 -115
==========================================
- Hits 35448 35333 -115
Misses 715 715
Flags with carried forward coverage won't be shown. Click here to find out more.
|
def get_sections(settings: dict) -> list[str]: | ||
layout = (settings.get("layout") or "").strip() | ||
if not layout: | ||
return [] |
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.
I think if you return nothing the PR comment would be empty.
It's better to return the default value in case there is none.
The default value is here (and it should not be lost when the YAML is processed, but merged with the user's option - I think that's the real bug)
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01%
==========================================
Files 442 441 -1
Lines 36163 36048 -115
==========================================
- Hits 35448 35333 -115
Misses 715 715
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Closing this, as this was only happening in QA for our own repos, and The error is caused by |
This accounts for the case when
settings
does not have any"layout"
.Also cleans up the code a little bit, using newer type annotations, and removing statsd timers.
Should fix ECDN-WORKER-D7G
fixes https://github.com/codecov/internal-issues/issues/974