On the topic of: Github PRs

[S34N]

Preface:

This is not really a "suggestion" as of such, more a problem the community is concerned about. Since there is no real place for such a public discussion about this specific topic, I have stolen the suggestion forum for it.

I want to make it crystal clear right here that this is NOT intended to bring up any ill-will towards the staff team, and we all agree that the work they do as volunteers is massively appreciated. Everyone has a life, we don't expect you to full-time care for Paradise. However, some of these points do involve asking hard questions to staff, and we want a civil but open discussion about them.

 

The issue at hand:

So, what's this all about? Why am I reading this forum thread?

Well, if you mosey on over to the Paradise github, you will notice we are sat at about 200 active PRs. This number has been slowly climbing over the past few months and is starting to reach an unmanageable backlog.

Why is this a problem? Surely 200 PRs is good since people are actively contributing!

Well, yes and no. It's great to see active development and people taking part, but unfortunately we currently have more active development going on than staff are handling, leading to the increase in PRs that are sat there stale.

We have a LARGE number of PRs that have been sat stale (or unmerged) for many months, and a handful over a year old!

 

This puts people off contributing, it makes it stressful to have to maintain months old PRs with no feedback on where/why they are not moving anywhere, and quite franlky means the game is not getting as much love as it should from a coding standpoint.

 

What is the cause of the problem?

Well, quite frankly, this is what this thread is here to discuss. It's not entirely clear why things are the way they are, or why they are not being resolved.

Factors we are aware of:

• Heads of Staff currently have to actively approve any non-fix/refactor PR. If a PR gets overlooked or no votes, it just sits there forever. (See: https://github.com/ParadiseSS13/Paradise/pull/15139)
  
  Unfortunately, it has become clear that heads are not really able to give the time to sort through our massive PR backlog, with approvals being sporadic at best. Now yes, headmins are busy and we appreciate all the work they do in other sections, but with this issue ongoing for over 6 months, it's clear the situation is NOT going to resolve itself by being left as-is.

 

• "Stale" review requests.
  These happen when a code change is suggested by a maintainer, implemented, but the maintainer does not re-review the code. This issue sort of ties into the lack of active maintainers, with @AffectedArc07 literally carrying the maint role singlehandedly for all intents and purposes. (See: https://github.com/ParadiseSS13/Paradise/pull/15404)

 

• PR limbo.
  PR limbo happens when a PR is objected to by one person, but does not get any further approvals. This is also linked to both above issues. PRs like this just drift towards the back page and are not tackled or looked at. So they just sit there. (See: https://github.com/ParadiseSS13/Paradise/pull/15445)

 

• Stale PRs.
  Stale PRs are ones that have not been updated in a long time, and are just sat there. Other codebases handle these by allowing maints to close them at their discresion, however we seem to just let them sit there forever. (See: https://github.com/ParadiseSS13/Paradise/pull/15442)

 

So, how do we fix it?

Well, this is the difficult part. It requires a lot of change somewhere, and I'm not sure where to start tackling it. This is why an open discussion with the community regarding WHY these issues have been allowed to creep up, and HOW the staff heads are planning to resolve the issues.

Things I can think of that might alleviate the issues somewhat:

• More ACTIVE maintainers. We appreciate the work put in by all our maintainers, and we don't want them to feel this is an attack. But currently, AA is the only maint actually doing anything remotely maint-like. He is doing everything and it is, no offense to him, too much for one person to stay sane and handle.
• Allow heads to object, but remove the requirement for their explicit approval on feature PRs. Let the good judgement of the maintainers (people who are familiar with the game and have a good sense of what will help or not) carry the code, and the heads can step in if they feel something is going to cause issues.
• Be more aggressive in closing stale PRs.
• Add a time-limit to review requests, if someone has been requested for a review and has not done so in months, their review should no longer be a hard requirement.
• Be more active in discussions regarding PRs in PR limbo, try to actively resolve them instead of just sweeping it under the rug and letting them accumulate in the background.

These are just a few solutions I can think of. Please, community, share anything you feel is relevant.

 

 

Lastly, we just need MORE involvement from the heads in the current system, and we should not let stale reviews/objections hold up the whole PR system.

@Kyet @necaladun @Dumbdumn5 I'm pinging you three here specifically, because I want each of you to please share with us your thoughts on why things have gone the way they have, and how you feel we can solve the issues.

 

 

[necaladun]

So as far as head approval goes - not an issue. We've approved about 100 or so. That's not an issue at all.

 

Really, there's one perfect solution to this: We chain up the maints in a basement on an IV drip of Amphetamines. But supposedly that's 'illegal' and 'unethical'. It worked great with Ponies tbh.

Look, frankly I'd love for the maints to be more active. I'd love more hours out of them. If you read this - please be more active and do more work for 0 pay and negative respect.

 

 But in the end, this is a volunteer project. The biggest thing that they've told me would help with this, is for the contributors and community to stop treating them like shit. Every. Single. Maint. Ever. Has complained to me about how toxic the github is, and how much they hate 'the communities' reaction to them moving rapidly.

 

If you want more shit done, give us the permission to do move quickly on things without the abuse, toxicity, and general bullshit we have to endure from it.

[Stokes52]

Hey, good thread. I've only just starting getting involved on the contributing side of Paradise, but I quickly noticed the backlog. Whenever I would ask on the discord what's up, the answer from others has always been some variation of "heads aren't approving things." which it seems is not the full story based on this thread.

I wonder if more transparency in the process would help.  As a new contributor, it would be nice to know:

1. What the process actually is.
I had to learn about the github approval process by asking around and I'm still not totally clear on how it works.  I understand that heads vote to approve features, but I don't know much beyond that. As far as I know the github merge policy isn't actually documented anywhere?

2. Transparency on the head votes/approvals.
I often see Kyet on github very helpfully approve or object in a comment, but I am never sure what that means. Is that him voting for it? Does that mean it's approved to be merged by all the heads? What else needs to be done? Do the other heads need to vote as well?

3. A clear indicator of when the baton has been passed to the next stage.
Maybe this can be accomplished with PR tags or comments, but its not clear to me who "has the baton" on most PRs.  Is it awaiting head review? Is approved and awaiting code review? Does it need changes?  Has it been objected to? It would be nice to know clearly what stage a PR is in. Sometimes github comments show this, but other times I hear through discord that the story is different on certain PRs. For example, I got a PM from Neca saying he voted for one of my PRs (thank you!), but based on what's in github I don't know what the other heads think, if it has been approved, if they need something more from me, etc.

4. Is it okay to PM people about github stuff?
I imagine staff get pinged all the time about random stuff. I don't want to add unnecessarily to the noise, but I'd also like to know how staff/heads/maintainers feel about discord PMs on github issues. Is it okay to ask what the status is or would you guys prefer we keep things in github comments? Is there already too much noise for you guys or is it okay?

The impression I get from discord is that many contributors feel their work is being ignored, but maybe that's not the case. Some of the suggestions above might help people see "Hey, we're looking at it, we just haven't been able to move it forward yet." Or, at least it will help people understand what stage their PR is in.

Related to maintainers:
Like I said, I'm new to the contributor side of things but I've already seen what a madman AA07 is and the work he puts in. A big thank you to everyone who contributes and maintains the github. It's often thankless work and I'm sorry you guys have had to deal with any abuse at all. You make the game possible and we players really appreciate it! It's unfortunate that angry players are usually louder.

I don't know what the process is for this, but is there any way we can make it easier for maintainers? Are there any PR reviewers that can step into the role of trial maintainer? Are there any contributors that are ready to step into the PR reviewer role? Is there anything the average player/contributor can do to make their lives easier?


Lastly, big thank you to the staff here. Paradise is a really amazing server and the work you guys put in has made something special in SS13 history IMO.
 

Edited July 1, 2021 by Stokes52

[SteelSlayer]

2 hours ago, necaladun said:

Look, frankly I'd love for the maints to be more active. I'd love more hours out of them. If you read this - please be more active and do more work for 0 pay and negative respect.

While it would be nice to have the current maintainers suddenly stop being busy and have free time to maintain the codebase, that's a silly expectation. IRL comes before SS13. But, The obvious solution is to hire more maintainers to spread out the work load (S34N already mentioned this). Then, in event some of them do get busy, the entire GitHub doesn't screech to a halt, and people wouldn't get frustrated as much, because things would be flowing. We have volunteers that want to do this, but it seems there is 0 care or effort into making this happen.

Overall good thread, you basically hit the nail on the head with all the issues we're facing. This stuff has been going on for years, so I've basically given up trying to talk or do anything about it, because it's usually brushed aside. I've already highlighted the biggest issue I think we have with the paragraph above. All I can do is hope things change.

[LightFire53]

20 minutes ago, SteelSlayer said:

The obvious solution is to hire more maintainers to spread out the work load

Only problem with this solution is that maintainers are relatively high up in the staff structuring, it's not as simple as pulling a PR Reviewer/Code Junkie or two aside and promoting them, as maintainers end up with admin access. In short, it's a coder with admin powers and there is a reason we have applications for admins to begin with. That said, I'm not fully privy to what the maintainer qualifications are, so I can't say that it's impossible to hire new ones, it's just more than you think.

[SteelSlayer]

4 hours ago, LightFire53 said:

Only problem with this solution is that maintainers are relatively high up in the staff structuring, it's not as simple as pulling a PR Reviewer/Code Junkie or two aside and promoting them, as maintainers end up with admin access. In short, it's a coder with admin powers and there is a reason we have applications for admins to begin with. That said, I'm not fully privy to what the maintainer qualifications are, so I can't say that it's impossible to hire new ones, it's just more than you think.

That's fair yeah. But I don't think anyone besides the Heads or Maintainers actually know the maintainer qualifications. The only solid information I've heard is "Be popular with the maintainers". Other than that, I have no idea what the requirements are, what knowledge you need, or how the hiring process is done. As far as I know, none of this has ever been shared with contributors/PRRs, but If it has, I'd love to hear it.

Edited July 1, 2021 by SteelSlayer

[Stokes52]

Is it reasonable to create a "Trial Maintainer" or "Junior Maintainer" role who doesn't get any other admin privileges? Just something to relieve pressure from the main maintainers.

This is a volunteer effort after all and the volunteer community wants to help.

[Kyet]

S34N:

• ~59% of all PRs have a head approval. There's ~21% another of PRs that are either pure fixes or refactors that either way already don't require head approval. Thus, the chances of head approval being the thing holding up any given PR is less than 20%. Approximately 63% of head-issued approvals for PRs are issued by me.
• Removing the requirement for heads to approve feature PRs is not an option. That would just put even more of the work onto the maints. As is, we serve as a sort of filter that stops maints wasting time with things that obviously won't work for design reasons. The requirement for our approval lets us reject those so we prevent them wasting maints' time. We also buffer some of the toxicity from players, so they don't have to deal with it.
• Ideally we would appoint new maints, but its almost impossible to find candidates for the position of maintainer that are acceptable enough to all the existing heads/maints that they can actually be added. I went on somewhat of a crusade during my two years as head+host trying to get more maints appointed, and even then I was only able to get one extra maint (AA) in the end. Politically, getting a maint appointed requires an extreme level of agreement among not just the heads but also all the existing maintainers. It is an incredibly difficult process. I've always been keen on the idea of junior maints but frankly with all that has been going on in my real life lately I quite simply don't have the energy for such a difficult task right now.
   

Stokes52

• In general, getting a feature PR merged usually requires (A) one head approval (B) one maint approval and (C) no heads or maints objecting.
• When I post a review of a PR that has a green check mark / marked as approved, I am saying that I am voting for that PR in my capacity as a head. Now it requires only (B) and (C) to be merged. I use my reviews to make my votes public. I have tried to encourage other heads/maints to adopt this system too, but most of them don't want to, partly as they dislike dealing with backlash on github. They don't like having to explain why they object to PRs, because they don't want to deal with PR authors messaging them "what do you mean, my code is terrible" or "what do you mean, you don't need another X in the game".
• When I post a non-approving review with the word "objection" in it, I indicate that I am voting against the PR. A head or maint voting against a PR doesn't automatically kill it, but it does make it MUCH harder for it to pass. Generally it can only pass at that point if another head votes for it regardless, a maint votes for it too, and a discussion is held among the heads/maints which ends with the resolution that it will still be merged despite the objection of at least one head or maint.
• As for progress indicators, you should probably assume that if it has a head's review/comment explaining that they like the PR, then its waiting for maint approval. If it doesn't, it is still waiting for head approval.

[S34N]

Okay, so since the headmins seemingly are approving PRs at a rate greater than I was lead to believe, that mitigates a lot of concerns surrounding the first of my known issues. Thanks to Kyet and Neca for your input and time looking at this issue. So, that leaves:

• "Stale" review requests.
• PR limbo.
• Stale PRs.

(my definitions for these are in the original post)

So next comes a ping for @AffectedArc07 since it seems these are more maint-oriented issues than headmin ones.

Are there any current plans or ideas to deal with PRs that have long time review requests or objections that have not been resolved in months?

Some examples of PRs in these categories for context:

https://github.com/ParadiseSS13/Paradise/pull/14335

https://github.com/ParadiseSS13/Paradise/pull/15445

 

Edited July 6, 2021 by S34N

[AllStickySocks]

On 7/2/2021 at 2:57 AM, Kyet said:

When I post a review of a PR that has a green check mark / marked as approved, I am saying that I am voting for that PR in my capacity as a head. Now it requires only (B) and (C) to be merged. I use my reviews to make my votes public. I have tried to encourage other heads/maints to adopt this system too, but most of them don't want to, partly as they dislike dealing with backlash on github. They don't like having to explain why they object to PRs, because they don't want to deal with PR authors messaging them "what do you mean, my code is terrible" or "what do you mean, you don't need another X in the game".

Would you be opposed to some type of labeling system that allows heads to remain anonymous while still giving a sense of feedback about what is happening to a PR? Or is anonymity a deal breaker? @Kyet

[SabreML]

Personally, I would be happy just knowing if one of my PRs has been objected to at all.

90% of the time it's communicated very well and the reasoning behind the objection is laid out clearly. On that other 10% though it can be incredibly frustrating to have a PR closed without even knowing that there was a single objection.

In some cases like the 'Genderless' PR I made a while ago then it's obviously understandable that people would rather not put a big, publically visible "I dislike this" on something as sensitive as that. But on other non-controversial ones, sometimes the most feedback you'll get is a  with no explanation, if even that.

I'm not saying this to complain of course, it's pretty obvious that all of the Maintainers & Heads are very busy people in real life who simply don't have the time to write a 2000 word essay explaining why replacing Tajaran with Felinids is a bad idea.

Even so, I would argue that the "If you can't justify it in words, it might not be worth adding." policy on pull requests should go both ways.
If they are unable put into words their reasons for objecting (Which again to reiterate, is completely understandable), then I don't believe that it should be officially counted.

That's just my thoughts on it anyway, at least in terms of objections.

Edited July 11, 2021 by SabreML

[Esenno]

On 7/6/2021 at 10:05 AM, S34N said:

So next comes a ping for @AffectedArc07 since it seems these are more maint-oriented issues than headmin ones.

I get it. It sucks to have to wait. But I feel pinging AA is kinda unnecessary. And asking him to devote more work to this server is kinda just unfair. Asking any maintainer to devote more time is also unfair. This is us pinging the coders saying to code more for free. Or the mentors to devote more time to the server. Just be patient instead. Besides I'm fairly certain the main issue behind the maintainer hold up has been addressed. And I for one wouldn't be comfortable with lowering standards just so we can get more maintainers.

[S34N]

3 hours ago, Esenno said:

I get it. It sucks to have to wait. But I feel pinging AA is kinda unnecessary. And asking him to devote more work to this server is kinda just unfair. Asking any maintainer to devote more time is also unfair. This is us pinging the coders saying to code more for free. Or the mentors to devote more time to the server. Just be patient instead. Besides I'm fairly certain the main issue behind the maintainer hold up has been addressed. And I for one wouldn't be comfortable with lowering standards just so we can get more maintainers.

This thread is for discussing these issues, and it is a conversation we need to have, it is not a thread to attack people and bitch about the work that IS being done.

Having said that, "Just wait" is the exact attitude that has contributed to this problem. It has gotten us to the point where we have an entire page and a half of PRs that are at least 6 months old.

Quote

Asking any maintainer to devote more time is also unfair. This is us pinging the coders saying to code more for free.

...

Just be patient instead.

I don't quite understand where you have got this idea from, but not once have I implied or demanded that maints should just "do more work." Nor does this thread come about due to some self-entitled opinion that my PRs should be merged. On the contrary, this thread was made because of the concerns the entire contributor community have regarding the long-term state of the project.

This thread isn't here to moan about things and throw a tantrum, it's here for a lasting public discussion that isn't buried in shitposts like you would get on discord. I would have pinged all the maints but I'm unsure if they use these forums, hence why AA got it. I'm sure he has or will discuss it with them regardless.

Now I know from discord conversations with AA he does not think this situation will continue for much longer, but those posts are buried and lost. The forum is a much, much better place to have this sort of rational discussion about the issues at hand.

 

To put the timescale of some of these PRs in context, some other codebases mark PRs as stale after a WEEK of no activity. Most of the open PRs on /TG/, for example, being from within the last month, with the oldest being from April. Our oldest is from February of last year, 2020.

[Kyet]

On 7/9/2021 at 1:03 PM, AllStickySocks said:

Would you be opposed to some type of labeling system that allows heads to remain anonymous while still giving a sense of feedback about what is happening to a PR? Or is anonymity a deal breaker? @Kyet

I don't object to the addition of a labeling system, but I am also very skeptical it would help.
How exactly would it make life easier for the maints/heads, bearing in mind that heads cannot label PRs? You'd need a convincing answer for that.

 

On 7/9/2021 at 2:14 PM, SabreML said:

I would argue that the "If you can't justify it in words, it might not be worth adding." policy on pull requests should go both ways.
If they are unable put into words their reasons for objecting (Which again to reiterate, is completely understandable), then I don't believe that it should be officially counted.

Intuitively, I agree with you.

In practice though, you're not taking into account (and historically, I have failed to take into account) that:

• maints can get an enormous amount of harassment over even normal PRs, and this is even worse for PRs they object to. AA just did an announcement within the last few days complaining that he got 14 DMs in the space of a few *hours*. Imagine dealing with dozens of DMs per day, every day, about PRs. In practice, you would quickly get super tired of this and it would make you re-think your willingness to publicly state objections to things if you thought it might lead to yet another hour of being deluged in DMs.
• maints are volunteers, and there's a limit to the amount of free time they can afford to spend doing free work for paradise without it hurting their real lives. While it may sometimes be frustrating for PR authors, ultimately the more time they spend arguing with authors over why their PR does not deserve to be merged, the less time they have for actually merging other things, and the worse the PR backlog gets. Sure, in theory simply stating their objection won't become a long argument, but in practice it often does.
• there are some extremely toxic people on github who will raise UNHOLY HELL about even trivial, terrible quality PRs being rejected. Such people are incredibly energy-draining to deal with. The maints having to spend time on those people is demoralizing.

I've been told this week that the reason I get away with having a "always explain my objection reasons on the PR" policy, and don't get harassed as a result, is that people fear me.

Unsurprisingly, I mostly disagree. For one I don't think most PR authors fear me. For another, I think that by stating my objection reasons publicly, I have demonstrated I am quite willing to defend my position, and that makes people less willing to challenge me over it. There are also other factors, such as the fact I think maints get a lot more harassment over PRs in general than heads do. Another possible factor is personality differences. Essentially most of the maints and some of the heads dislike coming into conflict with other people, and really don't enjoy having to argue. Deep down, they want agreement. In comparison, not only am I totally comfortable with disagreement, I even enjoy arguing at times (I spent a fair amount of time in college debate clubs, arguing for fun). I'm also used to operating in environments where consensus is impossible, yet decisions have to be made, so you have to accept some pretty sharp argument (and the fact some will leave unhappy) as the price of admission.

All combined, this means that most of the maints/heads are a lot less willing to explain their objection reasons than I am. And to a large degree this is understandable. While I'd certainly *prefer* they explained all their objection reasons in public like I do (it hasn't always been easy, but I do believe it gets easier the more you do it).... in practice that just may not be doable for them, and certainly not without asking them to go against their instincts, natural way of doing things, etc. And while it might obviously be fairer for PR authors if all objections were accompanied by a reason... in practice if I had to choose between the other heads/maints always giving an explanation for their actions... and y'know... actually getting the work done... obviously I'd prefer they actually get the work done.

So I guess what I'm trying to say is... we can't force the other heads/maints to always explain their objections. And even if we could, the time that would require might be counter-productive. What we can do is reduce the cost they perceive for doing so, largely by making sure that people aren't allowed to harass maints over their objections. Which we are doing. Via the new guidelines on github.

So yeah, I sympathize with your point of view. Still, instead of looking at it from an "is this fair to me as a PR author?" angle, I would encourage you to instead look at it from an "given the limited time/energy/morale/etc of the maints/heads, when is it worth, or not worth, explaining an objection?" angle. Because while sometimes it clearly is in the best interests of the project for it to happen... other times... its just sapping maintainer/head morale for no benefit.

[S34N]

12 hours ago, Kyet said:

I've been told this week that the reason I get away with having a "always explain my objection reasons on the PR" policy, and don't get harassed as a result, is that people fear me.

Whoever told you this is having a laugh, this may just be me but you aren't some scary eldritch horror that will consume whoever PMs them. You are approachable to anyone that can take a straight answer in their stride. <3

 

12 hours ago, Kyet said:

there are some extremely toxic people on github who will raise UNHOLY HELL about even trivial, terrible quality PRs being rejected. Such people are incredibly energy-draining to deal with. The maints having to spend time on those people is demoralizing.

Do we want these people in our community? If this happened in-game they would most likely face administrative action. While speaking out against staff should never be the sole reason for someone being restricted in the community, extreme toxicity very much should be dealt with. Hopefully the team will be able to more confidently deal with these then the repository code of conduct is merged.

 

12 hours ago, Kyet said:

Essentially most of the maints and some of the heads dislike coming into conflict with other people, and really don't enjoy having to argue.

I think this here is a key point. The word harassment gets thrown about a lot, and it has become a little murky. What a lot of players want is to "change the opinion" of whoever objects. As you said its natural to want agreement. The issue is when everyone PMs regarding their PR, it's just one PM for the user but 173 for the maint/head.

I personally feel that conversations regarding objections should always be held in public, say on the PR itself, as opposed to in DMs. This lets the community clearly see both sides of any discussion that does happen, and would reduce any cases of harassment as people on a public forum are much less likely to be toxic.

Perhaps the heads/maints could have an "autoreply" template asking people to host discussions over objections in the PR comments?

 

 

It's becoming clear that this topic as a whole is an extremely complicated one to resolve, and I appreciate the communication from staff to the community regarding it.

Edited July 13, 2021 by S34N

[Viz]

3 minutes ago, S34N said:

Perhaps the heads/maints could have an "autoreply" template asking people to host discussions over objections in the PR comments?

Draconian as it is, I think this combined with "ignore the warning at your peril" kind of policy where you get blocked for trying anyway would be a good thing. Entitlement and toxicity should be fought back against.

One thing I would like to point out is that until quite recently, the community at large had no idea about the toxicity issues maintainers are facing. With this fact becoming public, many will now be more wary of this happening, and I would expect at least some change.