Conversation
doismellburning
left a comment
doismellburning
left a comment
There was a problem hiding this comment.
My comment aside, this broadly looks reasonable, but I'd be wary of hitting "Approve" without some actual computer testing / linting etc. - I definitely haven't done any of that myself
|
|
||
| return change.success() | ||
|
|
||
| if not repo.allow_auto_merge: |
There was a problem hiding this comment.
Why this check? Doesn't this mean we'll only create a Change if the repo doesn't already allow AM? But surely we still want to if it does and we're unsetting it? Plus this makes the if in 364 always False, no?
neither have I. 😬 And you're right, this code is sus. The reason for it is that GitHub Copilot dutifully created a function with the exact same pattern as the one above, which has the same logic bug. Together with #17 this should make more sense now. I'll still test it in a quiet minute tomorrow. |
2 participants
No description provided.