Resolved: Development is now even slower

[tcharding]

We are attempting to find a way to keep track of all review suggestions and make sure that things that should be actioned get actioned. As part of this we introduced a no-merge-with-unresolved-threads rule. This has had the effect of making development even slower, now if a PR gets an ack from dev A, has an unresolved thread from dev B, and then gets an ACK from dev C, we cannot merge. This means we are no longer using "loose consensus" but rather "everyones opinion matters". I personally don't think every opion on every matter must be worked out with total consenus - development here is already painful enough, I don't believe it needs to be.

I think we need to iterate more on our merge policy / reaching consenus. Way too much bickering about trivial shit, would we be better off if we focus more on making progress and less on making every minute detail perfect right now? I"m not pointing fingers, we are all at fault here but we are all grown adults lets sort this out.

[apoelstra]

Agreed. I think we should drop the "must resolve everything" rule. I have also been quite vocal in complaining that the "resolve" button hides things.

[tcharding]

IMO we should go back to

• all review suggestions must have either a "yep sure thing, done as suggested", "Did not do for reason XYZ", or "created issue: #XXX "
• dev can click resolved if its fixed and was trival
• re-ack by reviewer implies the last iterations discussions are all resolved. Onus on reviewer to check if they really care how it was done
• dev to make best effort at not forgetting things

[apoelstra]

All sounds good to me.

[apoelstra]

For reference, our current process, as I understand it (since like 2 days ago) is:

• Any open discussions must be resolved to merge. Github enforces this.
• If anyone starts a discussion, they may resolve it.
• If a non-maintainer starts a discussion, any maintainer may resolve it.
• If the discussion is purely a style nit, the PR author any maintainer may resolve it.

The idea here is that if somebody opens a discussion they should be the one to close it, since they're the judge of whether the problem was fixed. With some escape hatches for maintainers.

A discussion can be considered "resolved" when:

• The problem in question has been fixed;
• An issue has been opened about the discussion, and linked in the PR thread (not just in the discussion itself, which will disappear when "Resolve" is clicked); or
• The problem is a trivial style/formatting nit.

[apoelstra]

However, despite all these escape hatches, we've repeatedly had PRs held up by this. In particular @Kixunil has repeatedly "ACKed with comments", which then holds up the PR and is worse than not ACKing at all. Since me, Kix and Tobin are all roughly 8 hours apart this often requires at least one 24-hour cycle to deal with. In some instances these comments haven't even been actionable.

So I think we should drop the Github "comments must be resolved" rule and go back to @tcharding's suggestion.

[Kixunil]

Well, the thing is this rule actually makes me more open to acking things I would've have acked before because I am now confident none of my comments will be unnoticed. I can't currently tell you which specific instances this changed in practice but I at least feel more at ease in my mind.

[Kixunil]

Is it really that bad though? The comments have to be addressed anyway, the only difference now is we won't forget about some. I believe you're not proposing "let's ignore comments" policy. For those which are within my ACKs I already gave permission to click resolve. For me ACK means may be merged. My final comments are either "this can be better and might be nicer to rewrite the commits rather than making a separate PR" or "additional, maybe unrelated thoughts". Usually if a PR has two ACKs merging is preferred.

[apoelstra]

Yes, comments need to be "addressed" before merging. Sometimes that means changing the code. Sometimes it means opening an issue. Sometimes it just means everyone acknowledges that there is something worth acknowledging that we should be aware of (and that future reviewers should see). Sometimes it means acknowledging that there's something unimportant that the PR author ought to fix if it's not too much trouble.

Only in the first case does it make sense to hide the discussion forever, and in none of those cases (except maybe the style nit one) is it clear what to do when the original discussion opener has gone AWOL.

[apoelstra]

I have disabled the "resolve conversation" requirement for merging. This has held up a ton of PRs, frequently for multiple days, and spawned a ton of navel gazing about what it means for something to be "resolved".

IOW way more actual pain that was caused by, say, the weekly format bot or the API diff requirement, which both have been overridden because a single developer was put out by them.