PR Procedure

[samiebee43]

I need to do more manual testing before submitting PR's. Recently #135 I made two commits that I thought would fix a thing that did not actually fix the thing.

Furthermore, whenever I get a PR from a different WP repo, uncertainty about whether I should pull it down, get acquainted, configure build, and test slows things down.

So I want to get on the same page about the role of our PR's and code reviews. Namely, PR should be after manual tests, and authors should perform manual testing. In the specific case of Enlighten, it would be sensible to continue our practice of testing before PR and again during review.

Today I've been trying out a practice of including code quality remarks in code reviews. I think this is one way to control the health of the repo in gradual steps, although it may mean rolling in some corrections that are not even part of the diff (might be annoying).

Kicking off this discussion for this initial point and anything else that may improve our PR experiences.

[mzieg]

My practice / expectation is that the submitter / author is presumed to have tested the change in their own environment. I absolutely do not expect reviewers to build and test every PR — in many cases, reviewers won't have the required hardware (R&D prototype, XL, SRM sample, calibration lamp, sometimes even compiler / toolchain).

I actually think "code quality" comments are the correct and expected level. In essence, "Assuming this works (you wouldn't have posted the PR without verifying it seems to work), here are some thoughts on how it might be done better / cleaner." Sometimes, "Have you considered these other use-cases."

I do not hold the reviewer to the standard, "if something goes wrong, and you approved the review, you are at fault as much (or more) than the author." I primarily use pull requests as "a second pair of eyes" to spot easily-identified issues in design and documentation, and perhaps more importantly ongoing cross-training so at least one other person is "grossly familiar" with our various repositories and libraries.

I accept that the above quality standard allows bugs to occur undetected within our code. In a nutshell, that's partly why all our code is free — it is not guaranteed to be bug-free, and as a company, we will sometimes need to prioritize development speed / timeliness over perfection. That is life in R&D.

I like the proposal to address "code health" in gradual steps, and yes it is valid to say "while you're in this code, could you make these other general improvements." Just be aware that sometimes the answer will be "no, I only scoped this task for this particular change, and if we need to broaden the scope and add time, we need to do that in a different ticket under appropriate prioritization."

I also think it's important to distinguish between "release blockers" and "suggested improvements." Both GitHub and BitBucket have a "Changed Requested" option, which I treat as "I am withholding review approval unless these mandatory changes are made." Otherwise, I treat comments as "suggestions" which the author has the freedom to disagree and decline.

[mzieg]

I guess I'll also add, my threshold for approval / merge is usually "does this make it better than before," not "is this perfect."

[samiebee43]

Thank you for your input.

ongoing cross-training so at least one other person is "grossly familiar" with our various repositories and libraries.

Definitely this was an outcome of the last VCPP PR.

Both GitHub and BitBucket have a "Changed Requested" option,

Good to know. This option is accessed by clicking "Start Review" instead of adding single comments. Then when you click "Finish Review" the option is there.

I accept that the above quality standard allows bugs to occur undetected within our code. In a nutshell, that's partly why all our code is free — it is not guaranteed to be bug-free, and as a company, we will sometimes need to prioritize development speed / timeliness over perfection. That is life in R&D.

This aspect keeps me up at night. The shortest resolution is quality-control insofar as it goes for the sake of near-future punctuality, which I think we are tending towards. This corresponds to moving from the purple exponential graph toward the pink one. I have lots of thoughts about this, but I think I better save it for its own discussion thread-- the main point is not moving so quickly that we are forced to deal with bugs and architectural changes which ultimately slow us down. I think we are doing a good job so far of this, but there is more to be done.

[mzieg]

This aspect keeps me up at night. The shortest resolution is quality-control

I would love to get our regression tests cleaned up a bit. I try sometimes, but I admit it's almost never prioritized.

[samiebee43]

For approvals, do you prefer "Approve and Merge" or just "Approve" and let the other person merge?

There has been at least one instance where I had something approved then later rescinded it. #129

Arguably those cases are less likely to be rescinded if the branches are merged sooner and not become stale.
"Approve and Merge" is a bit faster,
"Approve and Wait" is perhaps less accident prone

[samiebee43]

We settled verbally on "Approve and Wait"

[samiebee43]

Including "Close" followed by a issue number in a PR (rather than a commit) has better integration with GitHub.

Specifically, it shows a PR icon next to issues that have been handled (or partially handled) somewhere.