-
Notifications
You must be signed in to change notification settings - Fork 7
fix: was_squash_via_merge_commit logic #388
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #388 +/- ##
==========================================
- Coverage 94.18% 94.18% -0.01%
==========================================
Files 1255 1255
Lines 46308 46311 +3
Branches 1455 1455
==========================================
+ Hits 43615 43617 +2
- Misses 2388 2389 +1
Partials 305 305
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #388 will not alter performanceComparing Summary
Footnotes |
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.
Check if the merge commit sha is equal to the sha of the head of the PR because in that case we should also move the commits on the PR to the base branch of the PR.
What's the context for this change? The rebased changes will have a different SHA, so moving these commits to the base branch will cause a mismatch with commit SHA and the actual commit on the main branch.
apps/worker/tasks/sync_pull.py
Outdated
@@ -420,17 +420,34 @@ def update_pull_commits( | |||
return {"soft_deleted_count": deleted_count, "merged_count": merged_count} | |||
|
|||
def was_squash_via_merge_commit(self, repository_service, pull_dict): |
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: can you also annotate the types of this function?
There's a case where the experiment was failing because the head sha and merge commit sha were equivalent. The previous logic in the experiment would count that as a squash, because it was only checking the # of parents, but the previous logic was correctly detecting that as not a squash merge. |
Check if the merge commit sha is equal to the sha of the head of the PR because in that case we should also move the commits on the PR to the base branch of the PR.
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.
Seems low risk given the feature flag. 👍
Check if the merge commit sha is equal to the sha of the head of the PR because in that case we should also move the commits on the PR to the base branch of the PR.