doc: Improve documentation around the ACK statement #22966

jarolrod commented at 3:43 AM on September 14, 2021: member

We currently use several undocumented ACK statement's in every day review, namely: tACK, utACK, and crACK. Because of their ubiquitous usage, it's time to properly document them. 📝

In this PR, we document these ACK statement variations. Additionally, this performs some slight improvements to the general documentation around the ACK statement. Also, adds a dedicated section, 'ACK Statement' which explains what an 'ACK' is. Currently, one has to decipher the meaning by looking at the "Conceptual Review" and "Code Review" sections.

Master: render

PR: render

doc: add ack statement section to contributing.md

The ACK statement is an important part of any review.
Our current contributing.md doc doesn't do the best job
at documenting it; you must look at the "Conceptual Review"
and "Code Review" sections to decipher what it is.

This commit adds a dedicated section for the ACK statement.
We also move the explanation for NACK from the "Conceptual
review" section to the new section introduced here.

e9c9fc4a11

doc: document ack statement variations

We use several undocumented ACK statements every day, namely:
tACK, utACK, and crACK. This commit documents these variations
and their meaning in our contributing.md doc.

8b34635dc2

meshcollider commented at 3:58 AM on September 14, 2021: contributor

I believe it was recommended to move away from using these in favour of longer comments. The meaning of these statements is not well defined. For example, what is the difference between a utACK and a crACK? And what's the difference between an ACK and a tACK?

sipa commented at 4:03 AM on September 14, 2021: member

@meshcollider I agree.

I'm not opposed to codifying more things if they're actually and unambiguously used, but I'm not sure that's the case. One person's tACK may mean "I built it and ran the unit tests"; another person's may include playing around with RPCs/GUI in testnet and trying to break it. Those are such wildly different scenarios that if we want it, the documentation should clearly explain what it means. But I fear that today people are already using them with very different meaning, so codifying it may just add more confusion.

Also: code-review ACK, does that imply a concept ACK or not?

jarolrod commented at 4:07 AM on September 14, 2021: member

@sipa @meshcollider: thanks for the input!

"One person's tACK may mean..."

That's true ... perhaps this is better left undocumented? I was thinking it would be nice to document this, especially for new contributors who see these statements everywhere. Feel free to close if not wanted.

jarolrod commented at 4:10 AM on September 14, 2021: member

@sipa:

"Also: code-review ACK, does that imply a concept ACK or not?"

Well, conceptual review should have occurred prior to code review according to our doc, so no 😊

DrahtBot added the label Docs on Sep 14, 2021

naumenkogs commented at 8:00 AM on September 14, 2021: member

@jarolrod what you could do is just decode that crACK means "code review ack", and then highlight that for all these aliases the meaning is rather ambiguous.

MarcoFalke commented at 8:10 AM on September 14, 2021: member

I consider almost every ACK a code-review-only ACK, regardless of what it says. Running the tests, which is already done by the CI, doesn't "upgrade" the ACK.

In case the reviewer has done testing that goes beyond what the CI did, it will need to be explained in detail. A simple tACK can't convey that.

michaelfolkson commented at 8:42 AM on September 14, 2021: contributor

Agree with most of the above comments. I don't think we should keep bouncing around merging #16149 discouraging use of utACK and tACK and then reversing that in this PR. We can always attempt to make the docs clearer though.

Also: code-review ACK, does that imply a concept ACK or not?

I would guess if you were a Concept NACK you wouldn't ACK the code (a code review ACK to me seems to me to imply you were comfortable with Concept, Approach phases) but we could make this clear in the docs. It is at least possible that a reviewer disagrees with the Concept, Approach (e.g. NACKed) but still wants to ensure there aren't bugs in the code and so does a code review.

what you could do is just decode that crACK means "code review ack", and then highlight that for all these aliases the meaning is rather ambiguous.

I'd rather people just wrote ACK BRANCH_COMMIT with Code Review either before or after it plus optional additional details on exactly what the code review entailed. I don't think the time saved by using the shortened crACK is going to be material in the long run. No one seems to use cACK or aACK to save on typing :)

MarcoFalke commented at 1:25 PM on September 22, 2021: member

Can this be closed?

fanquake commented at 1:44 PM on September 22, 2021: member

Yes I think so.

fanquake closed this on Sep 22, 2021

jarolrod deleted the branch on Oct 4, 2021

DrahtBot locked this on Oct 30, 2022