Define what approving a PR means

[GarboMuffin]

I don't really have the time or (more importantly) motivation to review extensions these days.

I've gotten the feedback that reviewers don't merge pull requests because there is no guidance when things are okay to be merged. So I'm trying to define that here.

Here's my idea. We define passing the automated tests + getting two approvals as fully sufficient to be merged. Now just have to define criteria for an approval, so I've written some guidelines and a couple templates to copy and paste into the review comment box. Check all the boxes, then you can approve it. Get two of those, then any reviewer can merge it.

Read the new document. It's not long. Give feedback to refine it. You'll be the ones following it after all

[SharkPool-SP]

The only thing that's really missing is a bit more reviewers.

Cubester, PPPUD, DNin01 specifically are typically busy with personal projects. Don't take this as a call out to remove them, not at all, but they make up a decent chunk of reviewers.

[SharkPool-SP]

Dm me if you want some decent candidates

[GarboMuffin]

Send me some names

[SharkPool-SP]

Send me some names

@RedMan13
@JeremyGamer13
@DogeisCut
@Gen1xLol

[RedMan13]

yeah i would like to do extension reviewing

[DogeisCut]

I'd be down for reviewing extensions!

[CubesterYT]

Looks great! I agree with SharkPool, some more reviewers wouldn't hurt.

[PPPDUD]

The only thing that's really missing is a bit more reviewers.

Cubester, PPPUD, DNin01 specifically are typically busy with personal projects. Don't take this as a call out to remove them, not at all, but they make up a decent chunk of reviewers.

I can confirm this. I agree that we need more reviewers, preferably people who are available during the school year.

[PPPDUD]

@S4IL21 You seem like an interesting person, and this could be a learning opportunity. Maybe you might like to try being a reviewer? You don't have to, but I think that it would be really helpful if you joined.

[Brackets-Coder]

These seem like good guidelines for what's generally acceptable.
I do think that for extension reviewers you should provide more detail as to the standard of testing you had to reduce glance-and-approve reviews.
I also tend to agree that the PR library is piling up and we do need more reviewers

I would also like to say that while I'm committed to being a part of Turbowarp for the foreseeable future, I cannot guarantee I'll be chronically online or will not leave the repository in the distant future when bigger commitments override my time for Turbowarp. In the past few weeks I haven't been as active here but I aim to be increasing my time spent on Turbowarp as my outside life gets less busy. I love how you're trying to make Turbowarp less GarboMuffin dependent so it can still run even when you're gone.

[DNin01]

This seems good. One more thing, it might be good to determine the value of a change or feature when reviewing it. For example, if a small, easily reviewable but obscure feature that not many people are interested in (e.g., no reactions on its corresponding issue), we might not want to approve it, just for the sake of not wasting time on/or adding too many features and instead focusing on what matters.

[DNin01]

Also might want to mention security.

[PPPDUD]

@GarboMuffin If an extension passes the review criteria but has some obvious failure not listed in REVIEW_PROCESS.md, do extension reviewers reserve the right to withold their approvals?

[PPPDUD]

If we add more active reviewers, can we increase the minimum approval count to 3? People are quick to approve things and it only takes two people who aren't thinking clearly to cause an issue for everybody.

[SharkPool-SP]

If we add more active reviewers, can we increase the minimum approval count to 3? People are quick to approve things and it only takes two people who aren't thinking clearly to cause an issue for everybody.

I think that depends on the amount of reviewers

[Brackets-Coder]

@GarboMuffin Will you still review existing and new PRs that require more critical attention? E.g, the bigger extensions, concerning security issues, sketchy extensions that "work" but achieve functionality in an uncertain/odd manner, etc.?

[GarboMuffin]

@RedMan13 yeah i would like to do extension reviewing
@DogeisCut I'd be down for reviewing extensions!>

Invited. Will consider other mentioned people if they ask.

For example, if a small, easily reviewable but obscure feature that not many people are interested in (e.g., no reactions on its corresponding issue), we might not want to approve it, just for the sake of not wasting time on/or adding too many features and instead focusing on what matters.

It's supposed to be covered by "I verified that the extension follows all acceptance criteria and guidelines in CONTRIBUTING.md". I went and put something in there about nothing too niche

Also might want to mention security.

I'll mention it

@GarboMuffin If an extension passes the review criteria but has some obvious failure not listed in REVIEW_PROCESS.md, do extension reviewers reserve the right to withold their approvals?

Of course. You can add more beyond just the copied and pasted checklist.

If we add more active reviewers, can we increase the minimum approval count to 3? People are quick to approve things and it only takes two people who aren't thinking clearly to cause an issue for everybody.

If people actually go through the checklist then hopefully they won't be that quick to approve

@GarboMuffin Will you still review existing and new PRs that require more critical attention? E.g, the bigger extensions, concerning security issues, sketchy extensions that "work" but achieve functionality in an uncertain/odd manner, etc.?

Maybe

If the block design is odd then don't check the box "I reviewed the block list and believe they are well-designed and intuitive". If you mean that the code is odd then don't check the box "I reviewed the code and believe it is well-written and easy to maintain or extend in the future, even without help from the original author"

[GarboMuffin]

2 reviewers also does effectively already mean 3 people since you can't approve your own pr

[GarboMuffin]

yolo merge

[Gen1xLol]

Helro i was suggested as an ext reviewer yes yes yes also if you got a notif from another account please ignore it im begging you

[S4IL21]

@S4IL21 You seem like an interesting person, and this could be a learning opportunity. Maybe you might like to try being a reviewer? You don't have to, but I think that it would be really helpful if you joined.

Sure

[Brackets-Coder]

The only thing that's really missing is a bit more reviewers.
Cubester, PPPUD, DNin01 specifically are typically busy with personal projects. Don't take this as a call out to remove them, not at all, but they make up a decent chunk of reviewers.

I can confirm this. I agree that we need more reviewers, preferably people who are available during the school year.

The people who are not still in their years of academics likely have a job, so really whoever you get is going to be busy some way or another. The way to make more activity and progress is to have all the reviewers turn into full-time Turbowarp volunteers (which is impractical to ask) or just add more reviewers (which is the more realistic solution).

[DNin01]

I think it's okay to have people go in and out of being reviewers, so if one person starts working full-time on something else, another can replace them (as long as each member is committed for a little while, like several months or more).

[CubesterYT]

We are NOT adding genix as a reviewer

[PPPDUD]

We are NOT adding genix as a reviewer

What's wrong with them?

[PPPDUD]

@GarboMuffin Your review process says to ping you early and often; should we do this for every extension, only extensions with severe issues, or some other group?

[Brackets-Coder]

@GarboMuffin Your review process says to ping you early and often; should we do this for every extension, only extensions with severe issues, or some other group?

It says that for things other than listed above in Review Process. The point I implied is that for most things the reviewers should be able to handle it on their own, but for specific niche issues that require Garbo's attention be sure to get his attention respectfully without spamming

[SharkPool-SP]

We are NOT adding genix as a reviewer

Why not