-
Notifications
You must be signed in to change notification settings - Fork 55
fix: handle merge_group github events #683
Conversation
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
thomasrockhu-codecov
left a comment
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.
logic is sound
| if branch and branch.startswith("gh-readonly-queue/"): | ||
| return branch.split("/")[1] |
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.
for clarity can we do
| if branch and branch.startswith("gh-readonly-queue/"): | |
| return branch.split("/")[1] | |
| if branch: | |
| branch = branch.split("/")[1] if branch.startswith("gh-readonly-queue/") else branch | |
| return branch |
if we get a branch with gh-readonly-queue, it comes from a merge queue and the actual branch name is located in the middle
| return branch.split("/")[1] | ||
|
|
||
| branch = match.group(1) | ||
| return branch or None |
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.
my worry here is if we depend on None to be meaningful versus ''. Previous logic doesn't account for empty string
thomasrockhu-codecov
left a comment
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.
@joseph-sentry this looks good, can we move it to prevent-cli though?
if we get a branch with gh-readonly-queue, it comes from a merge queue and the actual branch name is located in the middle