Code review process for pull requests

Continuing the discussion from Development workflow for GitHub:

@stolle just asked more or less the same in Slack:

Are there already plany about review process on github? Will we use labels like needs review and +1 +2?

So here we go, ideas and opinions welcome!

I guess a label needs review and maybe a merge ready could be nice?

And for review I guess +1 / +2 are fine, maybe as happened already we use some emoji for easier spotting?

:+1: -> +1
:-1: -> -1
:100: -> would be +2 (for lack of something better)

And no -2, if you really want to block something then find a emoji and text to make that clear :wink:

Do we want to still officially split between code review and verified? If so:

Code review :+1:
Verified :+1:

and if we do that then maybe two labels verification needed code review needed could be helpful if one of the two is fine already.

I’d assume that a :thumbsup: means I reviewed and verified so a distinction isn’t really needed. If anything is unclear, rather amend with some text. We could maybe use :point_right: to “mark” a “+0 review” that would then need some text to explain…

As far as a :thumbsdown: for verification is concerned, we should have Jenkins and/or Travis CI report back on PRs via the PR API to (at least partially) solve that.

One more thing… can the assignment of labels be restricted to members of an org/team? Then the labeling as merge ready could be limited.

regarding labels: I guess we won’t need a “work in progress” label, right? Because if something is WIP, you just don’t create a pull request?

On the other hand: if you thought it would be ready for review but after the first review realized that you need to go back to the drawing table, would you close the PR oder would you … set some label?

Then you (or someone) would give a :thumbsdown: review, no? I mean, we didn’t abandon Gerrit reviews in that case, neither.

… or something like :no_entry_sign: or :hourglass:️.

Regarding milestones: that would basically be “fix version”, right?

Actually we could decide to just close the pull request and reopen one at a later stage. Your fork with your changes is still there and you can continue to work on it…

In the Symfony Project, pull requests are required to have the following table as part of the PR message:

| Q             | A
| ------------- | ---
| Bug fix?      | [yes|no]
| New feature?  | [yes|no]
| BC breaks?    | [yes|no]
| Deprecations? | [yes|no]
| Tests pass?   | [yes|no]
| Fixed tickets | [comma separated list of tickets fixed by the PR]
| License       | MIT
| Doc PR        | [The reference to the documentation PR if any]

We could introduce a variation of that to our own rules.

They also use labels a lot, for example:

Status: Needs Review
Status: Needs Work
Status: Reviewed
Ready
RFC
Bug
Enhancement
Feature
Easy Pick

… and labels for various components.

I could imagine that something like the labels above makes sense for us as well. Maybe we can even synchronise labels somehow with Jira, so that if an issue is mentioned in the PR (which should be the case usually), some script sets labels according to what we have in Jira.

1 Like

I like the labels, I dislike the table requirement.

1 Like

regarding the table:

I don’t like that it’s a table, but we do need the information (and we required it with our Gerrit process already). They question is where to put the information.

“Tests pass” should be determined automatically, license should be clear (GPL for Neos, MIT for Flow), documentation should be contained in the PR.

“Bug fix” / “new feature” / “BC breaks” was communicated in the commit subject, question is if we continue to do that. We could modify it a little and write subjects like:

Bugfix: Make infinite loop less endless

or

(!) Feature: Use PHPLIB instead of Symfony Components

Right?

Sure. I was just thinking about how we did it “back then”. OTOH, it might be a chance to begin using “abandon” more often, which we used only seldomly.

Regarding “WIP” tag/flag - there is still some case where you have some idea on how to implement a specific feature/task and want to show it early to get feedback, where it should be clear that this is not merge-ready. Should those cases then be handled with simply linking to the feature branch in one’s own fork, or would it still be valid to already create a PR?

By now we have a WIP label in github. So I guess PR and adding that should be fine.

We should write down a (recommended) flow for doing code reviews, I guess. It should include at least

  • start with a conceptual review (unless it’s a trivia bugfix, of course)
  • nitpicks can be done, but if there is a fundamental issue with the PR, sort that out first!
  • if possible, fix nitpicks yourself, instead of just commenting on them
  • if needed, ask the author for push access to the fork, so you can fix them easily
  • or do a PR on the PR (cumbersome, but works)
  • add labels and keep them updated
  • if a PR has the “discussion” label, check and try to participate
  • even if you don’t know the code area affected, your feedback on concepts, new API, documentation, configuration can be very helpful!
  • if you assign a WIP label, be prepared that noone reviews your code - if it’s ready for review, don’t mark as WIP
1 Like

good idea, could you add it to Committers Guide? if that doesn’t seem fitting, add a new section to Code contribution guideline

I would like to add the third option for nitpicks to just merge the original change and push a follow up fixing the nits afterwards (which then could probably be merged as no-brainer).

  • if you assign a WIP label, be prepared that noone reviews your code - if it’s ready for review, don’t mark as WIP

This is still a no go for me. IMHO everyone should be curious about conceptual ideas and very early feedback is sometimes very helpful to get stuff out of WIP. I usually open PRs as WIP to get feedback…
I would rather come to the formula:

  • If you don’t want your code/change reviewed don’t start a pull request (yet). If it is really only halfway done code wait until the concept becomes clear to open a PR.

Same goes for failing style/tests. Both may block a merge, but the concept might be OK just that the code changes break a lot of tests, also there it would be great to get some early feedback on the change itself.

2 Likes

But that should be marked as Feedback, not WIP, no?