This is the list I’ve had in my head, but it’s great to have it spelled out here. Only one subtle deviation to ask about, though: @chris, @guessed and I (possibly others?) submit and merge PRs to our respective bindings without another set of eyes on them before the merge, which seems historically not to be a problem and has, I believe, real practical value. Nevertheless, I think it would be good to verbalize this arrangement so it’s not outside the rules. How do you feel about this, @teichsta?
I would love to have the four-eyes principle only on PRs against core components and perhaps on important bindings like mqtt or myopenhab on which regressions would impact a large group of people.
Ifor we want two reviewers even for new bindings (making it a six-eyes principle) i fear we will be blocked again.
I already try to wait at least one day for contributions from other reviewers before merging stuff. Maybe we need another tag on the issues, perhaps “review-vote” or something to alert other reviewers that another +1 or LGTM is being asked for?
Normally, if it’s a code change, I make a PR, then leave it a few days before I merge to allow others to have a look if they want (sometimes others find problems - thanks @watou). If it’s database entries, then I just merge. I don’t think it hurts to allow others to take a look, but for ‘simple’ changes I would agree…
One other (related) ‘rule’ I’d be interested in is for bundles that have a ‘main contributor’, that any significant changes should have that contributors ‘approval’ (the terms in the quotes may not be the best terms to use, so feel free to pick something different ). This is just to ensure that there’s someone to check that the overall concept/structure/operation of the bundle doesn’t get messed up by merging code that has been checked by 4 eyes who don’t understand the code. They may check the code is good quality, but may not understand how the binding works, and therefore may inadvertently merge code that isn’t best for the bundle…
+1. I would be willing to wager that lots of regressions have resulted, despite the best intentions, by those who don’t fully understand how the original code is meant to work.
How should we manage PRs related to core functionality if it’s appears the developer is not interested in doing a ESH port and OH1 down port? We have some quite old core-related PRs that appear like they will not be completed even for OH1.
when i suggested a second reviewer for every PR i didn’t expect them to do a deep and thorough review but rather a quick look just to avoid little slips like commented code, to much (or less) logging, mismatching severities references to absolut paths, stuff like that.
I’s suggest to ask some of the others directly (probably randomly - use the ‘@maintainer’ group) to ask for the review.
Also in these cases i would tend to ask somebody else for review for the above mentioned reasons. Also, it is (hopefully) not carved in stone that you “just” maintain one binding. Your help is always welcome also with other contributions (as you surely know and you helped a lot already).
as we follow the rule “OH1 core is frozen” we cannot proceed with such PRs then. Better let’s discuss each case individually in the according PRs rather than generally here.
). If it’s database entries, then I just merge. I don’t think it hurts to allow others to take a look, but for ‘simple’ changes I would agree…