feat(mergechecker): adding mergechecker interface with Check#69
Conversation
kevinlnew
force-pushed
the
kevin.new/merge-checker
branch
from
2e76584 to
a146ad4
Compare
kevinlnew
requested review from
a team,
behinddwalls and
sbalabanov
as code owners
| // mergeability of the request. A nil result does not guarantee | ||
| // that the changes will apply cleanly at merge finalization time. | ||
| // Returns ErrUnmergeable if the changes are not mergeable. | ||
| Check(ctx context.Context, request entity.Request) error |
There was a problem hiding this comment.
let's have it return true/false + error
using errors to control code flow should in general be avoided (even though Go sdk is guilty of it sometimes).
There was a problem hiding this comment.
Most likely in the future we'll have to enhance the result with some metadata anyways. For example, return back to user with the additional info what files are conflicting against what hash/request. The return object is probably justified to be a structure of some sort.
There was a problem hiding this comment.
@kevinlnew can you leave an edit for CLAUDE.md to check for future changes on this
There was a problem hiding this comment.
Yep I'll do that in this diff
|
I see now that I am contradicting earlier Preetam's comment. |
|
Sounds good, bring the bool back |
| // mergeability of the request. A nil result does not guarantee | ||
| // that the changes will apply cleanly at merge finalization time. | ||
| // Returns ErrUnmergeable if the changes are not mergeable. | ||
| Check(ctx context.Context, request entity.Request) error |
There was a problem hiding this comment.
Most likely in the future we'll have to enhance the result with some metadata anyways. For example, return back to user with the additional info what files are conflicting against what hash/request. The return object is probably justified to be a structure of some sort.
| ) | ||
|
|
||
| // ErrUnmergeable is returned by Check when the request's changes are not mergeable. | ||
| var ErrUnmergeable = errors.New("request is not mergeable") |
There was a problem hiding this comment.
I think this error is not needed now
| // Check is a fail-fast validation that optimistically assesses the | ||
| // mergeability of the request. A positive result does not guarantee | ||
| // that the changes will apply cleanly at merge finalization time. | ||
| // Returns ErrUnmergeable if the changes are not mergeable. |
There was a problem hiding this comment.
also not relevant, error should indicate that the merge checkimplementation can't do the check at all.
There was a problem hiding this comment.
I'm fine to leave this to the implementation to return a relevant error message. Do we need typed err here?
…l; Updated merge checker to use a result struct with bool nested
3 participants
Summary
feat(mergechecker): adding mergechecker interface with Check
What?
Check: explains how this is not a mergeability guarantee but a heuristic to advance the request or fail it fast.Test Plan
Issues