What's required before a PR gets merged?

[SirTechSpec]

This started as a response to #7738 but I realized I have some questions about the existing process that I'd like to know anyway. By "active" I mean "participating for either a few minutes a day most days, or 30+ minutes a week most weeks."

1. How many active members can do administrative work (tagging new PRs and issues, associating them with each other, closing solved/wontfix/unlikely issues, etc.)?
2. How many active members can "approve" changes in each category (missions/story, art, other content, UI, other code subtypes, documentation, etc.)? How many approvals are required for each category (if any)?
3. How many active members can merge changes?
4. How many active members are there whose disapproval would pretty much kill a suggestion? (By category, if appropriate?)
5. Do we have good tools (review queues, reports, bookmarked search URLs, whatever) for bringing things to the right people's attention?

More broadly, to what extent are we relying on consensus, and to what extent executive decisions?

Where consensus is relevant, I think it would be good to know what threshold constitutes consensus. Where executive decisions are required, it's good to know who has to make them.

Otherwise I think there's likely to be a lot of bystander effect where everyone is either waiting for "someone else" in the abstract, or someone particular, who may not realize that they're the blocker. Or, flip side, you get people merging something and someone else says "whoa, I hadn't had a chance to review that yet [and I think I should have been offered the chance, because either we didn't have enough consensus by my count, or I'm the one who makes decisions about this category of change]."

[warp-core]

What does it take? For one of the "devs" to feel it is ready for merging. This group of people is comprised of: amazonite, tehhowch, Pointedstick, W1zrad, Anarchist2, petervdmeer, quyykk, and Saugia.

1. Probably 15 to 20.

2. Anyone can approve a PR for any reason, but there are a group of "respected reviewers" who's reviews are recognised to be generally good by some people in the community.

3. The "devs" I listed above, though tehhowch has not been very active for a while, I haven't seen Pointedstick around much (though I think he is here more frequently than tehhowch) and Wizrad and Anarchist have also been less active recently.

4. This one will likely change depending on who you ask, and whether or not one of their proposals has been rejected and by who if so. Does the question assume overwhelming support for the proposal other than one individual? Split support and a single person acting as the tie breaker?

5. Yes

[SirTechSpec]

1: So between this and 5, that says to me that if the devs are in the mood to review/merge something, there's no reason they can't (set up a filter|adjust their existing filter) to only show PRs older than 24h, or that plus all PRs tagged [typo], correct? (Not saying it's a good idea or not; just wanted to rule out "no one will tag new PRs as typos, so if we do institute a time limit, no one will see them in less than 24h anyway" as a reason not to implement that distinction.)

2: So "approval" isn't a technical term for this project? It's basically just a more-visible thumbsup reaction? Good to know.

3: So there are maybe 4-6 people who are likely to merge a change on any given day, 9 tops (you didn't list endless-sky aka MZ, but I assume he's also on this list.) That definitely affects how formal a process and how many "rules" are required.

4: Honestly I was picturing that they commented negatively relatively early on in the process, after which people wouldn't bother to comment positively even if they personally liked the idea. But say 80% support. (To be clear, I don't mean this in a pejorative way - I'm thinking about MZ shooting down something that doesn't fit with the overall point of the game, or a faction lead/story lead saying something doesn't make sense with the fiction.)

[warp-core]

1. Yes
2. Basically, there's no requirement that a PR have some number of approvals, though, approvals likely give devs more confidence that a PR is more likely to be ready to merge. That might just be that it has been reviewed by others so, if the dev looks at it, they are less likely to need to raise many issues with it. I'd say "more-visible thumbsup reaction" is not an entirely unreasonable description of how approvals are used here, though perhaps somewhat reductive; generally a person will have looked more closely at a PR, either the line changes themselves or testing it, if they've approved it than they would have if they just thumbsup'd it.
3. Yes, that's about how many. I didn't mention MZ because he has not really been involved with reviewing or merging stuff for a long time now, though if he wished to, he certainly could.
4. Well, if a dev is opposed to a change, they are unlikely to merge it themselves, so another dev would need to merge it. Whether or not they do is up to them, I can't speak for how likely any one is to override the decisions of another, I don't think it's really happened, though that could just be that in any cases of one dev being opposed to something, the rest are as well.

[Zitchas]

Sadly, number 4 has happened. Very rarely, but yeah. There was a PR that one dev felt was ready to merge. Another dev wanted to review it first, but didn't actually review it. Eventually the first felt that more than sufficient time had passed to allow the other dev to do so, and went ahead and merged it. Other dev got angry and reverted it. First dev got fed up and went inactive and basically has only been seen very sporadically since.

Not a great situation.

[McloughlinGuy]

Sadly, number 4 has happened. Very rarely, but yeah. There was a PR that one dev felt was ready to merge. Another dev wanted to review it first, but didn't actually review it. Eventually the first felt that more than sufficient time had passed to allow the other dev to do so, and went ahead and merged it. Other dev got angry and reverted it. First dev got fed up and went inactive and basically has only been seen very sporadically since.

Not a great situation.

I think I know who your talking about, but saying it might be a bad idea.

[Zitchas]

As a general rule, in terms of executive decision making, we trust those with GitHub priviledges to use the utmost extend of their abilities without restriction. There's some discussion about this, and it's not consistently applied, but as a general rule (especially for the listed Developers), we trust them to do what they think is best without forcibly requiring approvals from anyone. In fact, Devs have a strong capacity to overrule pretty much everyone in pursuit of what they consider to be best for the game. For instance, there was a poll on Discord about whether or not the Bactrian should have angled gunports. The final vote was 423 people in support, 43 opposed. Despite such massive support, the Devs chose to go with straight ahead gunports, and that was the end of the issue. (just for the record, I'm not bringing it up here to discuss it, just to illustrate the point that we have a tradition of strongly favoring executive decision making, not, as some claim, being a democratic system.)

We implemented the reviewer teams (we have teams for writing, art, missions, balance, code, and lore; although some of those only have one person in them) to highlight people who have particular respect and experience in those areas. The idea was to allow Developers to move more quickly in areas that they don't have much experience in. For example, we have devs whose primary strength is not coding. But if one or more of the trusted reviewers from the code team have approved something, the idea is that Devs such who lack experience/confidence with code stuff should then be comfortable merging it.

In general, I'd say overall in 95% of our PRs, this system works really well, and I don't think there's any need for time limits, approval requirements, or anything beyond what we currently do. Our team is, overall, very cautious about merging things and rarely do so unless they are comfortable with the change and believe it improves the game. Most of the recent discussion (and, really, the entire point that the time restriction poll is a thing) is due to a very small number of PRs that made changes that some people felt either happened too fast, or somehow managed to get by without them having them having any input on it. For instance, the Nanachi missions didn't appear to have anything to do with the Ember Waste, but it turns out they tucked a small attribute onto a reward outfit that has significant negative effects on lore pretty much exclusively applicable to the Ember Waste. I didn't know it was there, and it could have been resolved at the time, but no-one thought to mention to me that it was there.

So the bigger issue is really that in a small number of PRs, people who think they should have had a say, didn't. Unfortunately, I'm not sure what the solution is. Giving everything a minimum 24h timelimit, for instance, would not have solved most of the "problem cases" in the past few months, as most of them were around for many days, weeks, even.

But likewise I don't think it is reasonable that a PR creator should need to memorize a long list of everyone in the community and what their interests are and hold up a PR forever until that list has been checked off. So I consider the blanket 24h requirement to be both overbearing and useless, but I also don't see much of an alternative.

We do have, for instance, "faction leads" listed in the credits. We could start by pinging them when a PR touches something related to their area. Those positions currently don't have any particular importance or specified... anything, really. They're basically just token titles of appreciation and suggestions that one should probably run stuff by them if it touches stuff surrounding their faction. There's no real hard requirement to do so, no real statement of how much influence or control they do or ought to have; although generally I think their opinions tend to hold a fair bit of weight.

As far as that goes, though, I'm not sure all the faction leads are active, and not everyone knows which of those real names corresponds to what github name to ping, either. And, like the timelimit thing, while pinging faction leads when something touches on their area of expertise might have solved one or two of the problem cases of the past several months, it would not have solved all of them. I'm not sure it even would have solved half of them.

The problem really boils down to the edge cases and specific problems; with the difficulty that we usually don't find out that they are problems until after they are merged. Often long past. And there's no real way to tell if something is going to be a big issue or not. Even size isn't really an indicator, since one of the "problem cases" of recent history was a mere single line with an effect that some people either don't even realize is there or assumed was a bug. While there have been PRs with 1k+ lines that haven't generated even the slightest amount of controversy. A lot of the really boring stuff with little impact on lore or gameplay (like cleaning up the map file, for instance, can easily run into the many thousands of lines.

[SirTechSpec]

Okay, cool. So, to summarize, devs will merge a PR if either:

• they're confident it's ready to go
• they're not sure because it's not really their area (of lore, code, whatever), but one of the Reviewers for that area has approved it. (Presumably, the reviewers are not themselves devs, or else they would have just merged it.)

That's... really good to know, thanks.

[warp-core]

(Presumably, the reviewers are not themselves devs, or else they would have just merged it.)

Indeed not. Anyone can review a PR, but some members of the community have been given "triage" permissions (editing labels, projects, assignments, requesting reviews, closing issues and PRs if appropriate, usually for PRs that have been abandoned/orphaned and issues that are resolved or duplicates), these people may be referred to as "reviewers" (or, "respected reviewers", which is probably less ambiguous) specifically because they have shown an ability to do good reviews (at least, I think that's the idea, MasterOfGrey largely developed the concept as we use it here.)

[SirTechSpec]

As for alternatives to the time limit, you said:

I don't think it is reasonable that a PR creator should need to memorize a long list of everyone in the community and what their interests are and hold up a PR forever until that list has been checked off.

Well, I certainly agree with that as far as it goes, but I also don't think that's the only way to go about it. If I were in charge of increasing oversight, I'd probably start with asking the devs not to merge things unless they've been signed off on by the relevant person.

To make that feasible, you'd probably want something like this:

• Make sure we have reviewer coverage for each "area" (both the current areas and the different factions): currently active and willing to serve in this capacity; not necessarily 1:1 and indeed, ideally there'd be 2+ for each area, even if that means combining smaller ones; may include devs, especially if they're lore/writing experts; not necessarily corresponding to the "faction leads" in the credits if that says more about historical contributions than current availability.
• Make sure we have enough taggers (i.e. people with permissions to do clerical work - ideally, at least some who are neither reviewers nor devs) to ensure that nothing languishes in untagged obscurity for too long.
• Adjust the PR-submission templates to explicitly include bits for "This PR contains Art and needs an Art review/This PR contains ships or outfits and needs a Balance review/This PR contains written content and needs both a Syntax and a Writing review", etc. These should automatically generate Tasks on the PR, to be checked off when completed by the relevant reviewers. And also automatically assign the relevant tags, come to think of it.
• Reviewers are asked to look at new PRs in a timely manner (whatever we decide that means) and approve or suggest changes, as usual.
• Devs are asked not to merge changes unless all tasks are complete - and depending on how strict we want to be, completed by someone else, so that there's always 2+ sets of (trusted) eyes on anything before it's included in main.

But of course, that's only helpful if it's actually helpful - that is, if always waiting until something is reviewed by somebody else before pushing it would fix (or at least reduce the frequency of) a real problem. And if either that's not the case, or there's not enough trusted reviewers to make it feasible, then it's not really time to increase oversight.

[SirTechSpec]

@warp-core Wait, what? "triage permissions" sounds very different from Zitchas's description of the "reviewer teams" above, except for the common element of being trusted members of the community. Are the same people doing both things?

[warp-core]

Yes. We don't have a whole lot of granularity with permissions on GitHub. Triage, I think, allows requesting reviews (which is the important part for reviewers, to request reviews by each other and also of devs when the relevant reviewers have reviewed a thing and approved it) but it also comes with the label management as well, so we end up doing that as well.