Proposal for PR workflow

Discuss general development subjects that are not specific to a particular version like the versioning control system we use or other infrastructure.
Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Proposal for PR workflow

Post by Oleg »

Currently we have 85 outstanding PRs.

A lot of them, over half in my estimation, are waiting to be fixed. The remaining ones are waiting for a review and merge.

Github provides no workflow for PRs to indicate this status. Their notification system is also useless in this regard.

Also, we have "unreviewed patch" and "unmerged fix" statuses on the tracker. Some of the unreviewed patches are e.g. inline code snippets that need to be converted to a PR before they can be merged. Other unreviewed patches are ready PRs which are no different from PRs submitted by developers that are unmerged fixes.

Proposal:

1. "Unreviewed patch" becomes a status for things which are not PRs, or for PRs that are not finished.

2. Replace "Unmerged fix" with something like "Awaiting review". This applies to PRs only that are, as far as submitter is concerned, finished, with all issues addressed. Both developers and non-developers would use this status.

2. Add "Patch needs work" status. After review PRs get sent there. I am not sure if this should be combined with "Unreviewed patch" or not, opinions are welcome.
User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: Proposal for PR workflow

Post by nickvergessen »

+1 and I would keep them separated, so you can distinguish between fixes that have a PR and fixes that dont
Member of the Development-TeamNo Support via PM
User avatar
naderman
Consultant
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Berlin, Germany
Contact:

Re: Proposal for PR workflow

Post by naderman »

I agree with your proposal in general. For clarity I propose that we simply change the current "Unreviewed patch" to "Patch awaiting review", and we make it our policy to mark them as "Patch needs work" whenever we find any faults with it during review. I don't think there is a need to have another "Unreviewed patch" status for non-PR patches, as the same two step workflow can apply to them.
Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: Proposal for PR workflow

Post by Oleg »

Non-developers cannot make unmerged fixes?
User avatar
naderman
Consultant
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Berlin, Germany
Contact:

Re: Proposal for PR workflow

Post by naderman »

We should just get rid of that step, it's either a patch awaiting review or after merging an unverified fix. There is no need for that step anymore.
User avatar
MichaelC
Development Team
Development Team
Posts: 889
Joined: Thu Jan 28, 2010 6:29 pm

Re: Proposal for PR workflow

Post by MichaelC »

With regards to implementing this it should be relatively simple to create a page on Area51 which lists PRs and allows people to change their 'status'

With regards to status, I'd suggest the following statuses:
1. Requires fixes by author - After awaiting review and items have needed fixed - Set by team members
2. Abandoned - When nobody is developing/fixing it - After 2 months of no activity, by PR author or team members.
3. Awaiting review - Development is mostly finished, needs a review - Set by author
4. Awaiting Final Review - Previous issues have been fixed, you think there is nothing else wrong and its ready for a final review & merge - Set by author
5. Needs testing - Title says it all - Set by author
6. Review whilst in Progress - Whilst being in active inital development,but can be reviewed whilst being worked on. True implementation of WIP PRs - Inital
Formerly known as Unknown Bliss
psoTFX wrote: I went with Olympus because as I said to the teams ... "It's been one hell of a hill to climb"
No unsolicited PMs please except for quotes.
Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: Proposal for PR workflow

Post by Oleg »

If you make people go somewhere from PR to change the status, said change might as well happen in the ticket. I will wait to comment on the additional statuses proposed.
User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: Proposal for PR workflow

Post by EXreaction »

Normal users cannot make changes to the status of tickets though?
User avatar
MichaelC
Development Team
Development Team
Posts: 889
Joined: Thu Jan 28, 2010 6:29 pm

Re: Proposal for PR workflow

Post by MichaelC »

EXreaction wrote:Normal users cannot make changes to the status of tickets though?
Neither can the make changes to labels in PRs.

A separate script is just a lot easier to use, it doesn't need to have more than one page with a list of filterable PRs, a status and buttons to change status. It doesn't need enough effort/time to worry about that side of things.
Formerly known as Unknown Bliss
psoTFX wrote: I went with Olympus because as I said to the teams ... "It's been one hell of a hill to climb"
No unsolicited PMs please except for quotes.
User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: Proposal for PR workflow

Post by imkingdavid »

I'm fine with what naderman and oleg came up with in IRC yesterday; two statuses: awaiting review and needs more work. As far as I know naderman can change permissions on who can use which statuses so that won't be an issue. This way, a user submits a ticket, it is new. Another user submits a patch and sets it to awaiting review. A developer reviews it and it isn't ready, he sets it to Needs more work, the user fixes it, sets it back to awaiting review, a developer reviews it and it is good, he merges it.
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.
Post Reply