improve PR reviews #5616

akosyakov · 2019-07-01T09:42:33Z

What it does

I want to start working on improving our review process. We have many PRs and I feel being a bottleneck in the process when many things can be checked without me. This PR proposes:

the PR template with a checklist which should be filled in before a PR can be merged. It does not matter by whom (contributor or reviewers).
more strict rules on approves to make sure that testing and design validation gets higher priority than human linting of empty lines. It should also make it easier for reviewers to judge whether some aspects already reviewed and improve knowledge sharing by nudging developers to test and review design.

Rendered PR guidelines

How to test

Review whether something missing, too strict or unnecessary in the proposed review checklist

Review checklist

as an author, I have thoroughly tested my changes and carefully reviewed following the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

.github/PULL_REQUEST_TEMPLATE.md

tsmaeder · 2019-07-01T12:51:30Z

I know it's a lot of work for you doing all those reviews, but I think it has served us well, so far. You have made sure the architectural integrity has been preserved. I fear the "design validation" could start to suffer. It's always hard to tell someone to start over when they've invested 2 weeks on the implementation. I think doing threads like this one https://spectrum.chat/theia/dev/how-should-we-go-about-register-monaco-commands-used-by-vs-code-extensions~2c74f56c-2d24-4f21-9db6-8041f0e861c9 will help with validating a design before we get too invested in them.

.github/PULL_REQUEST_TEMPLATE.md

kittaakos

Very nice initiative 👍 , I have added a few remarks on typos.

.github/PULL_REQUEST_TEMPLATE.md

akosyakov · 2019-07-04T07:28:09Z

Please have a look again. I've tried to address all comments and did following changes:

beginning of the review checklist clarifies responsibilities of an author and reviewers
following points are ordered according to the importance, starting from testing and design checking and finishing with proper git history
inapplicable items should be just marked as completed as well, crossing them out is big effort with the GH UI

akosyakov · 2019-07-04T07:29:34Z

I know it's a lot of work for you doing all those reviews, but I think it has served us well, so far. You have made sure the architectural integrity has been preserved. I fear the "design validation" could start to suffer. It's always hard to tell someone to start over when they've invested 2 weeks on the implementation. I think doing threads like this one https://spectrum.chat/theia/dev/how-should-we-go-about-register-monaco-commands-used-by-vs-code-extensions~2c74f56c-2d24-4f21-9db6-8041f0e861c9 will help with validating a design before we get too invested in them.

Agree that contributors should start discussions and open PRs early even if they are incomplete in order to verify the design first.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

kittaakos

Thank you for putting all this together! 👍

benoitf · 2019-07-04T10:01:34Z

it would be great to use https://www.conventionalcommits.org as well

akosyakov · 2019-07-04T10:18:26Z

@benoitf let's discuss it next dev meeting?

The goal of this PR to make existing review expectations clear.

Also @benoitf had some good points in #5622 (comment). I try to summarise here:

the PR should be minimal as possible or at least make unnecessary clean ups as separate commits
description should be complete
such review checklist can be long

Regarding the last point. It looks already long to me as well. I still think we should have it to make it clear about expectations in order to help new contributors to know what to look for. Maybe we better to add Reviewing Pull Request guide as a separate document and here have a short checklist like:

Review checklist

as an author, I have thoroughly tested my changes and carefully reviewed in accordance with [the review guideline](a link to guide here).
each approve has supporting comments in accordance with [the review guideline](a link to guide here).
- the approve without a comment should be dismissed

benoitf · 2019-07-04T10:38:28Z

@akosyakov sure I don't want to hijack the thread, let's discuss it on dev meeting

.github/PULL_REQUEST_TEMPLATE.md

akosyakov · 2019-07-18T13:53:29Z

Can we remove them and have a general section on how to do code reviews? Or is there a template for code reviews as well?

I've extracted a new document on how to collaborate and review PRs. The PR template is short now and references this document.

vince-fugnitto

The changes look good to me :)
I'm glad we took steps in order to improve the review process and make it easier
for authors as well as reviewers and document our process.

thegecko

Looks good, I've suggested a bunch of minor grammar amendments

doc/pull-requests.md

.github/PULL_REQUEST_TEMPLATE.md

doc/pull-requests.md

thegecko · 2019-07-19T13:54:48Z

doc/pull-requests.md

+
+<a name="reverting-pr"></a>
+- [1.](#reverting-pr) If a PR causes regressions after landing
+then an author has 2 days to resolve them after that a PR has to be reverted.


has 2 days

How will this be managed?

We will ask an author to look into a fix if there is no response and we don't have time to fix it ourself then we revert it. As it was done here: #5770

Also if an author is responsive and trying to fix, but we keep receiving bug reports from users as it was the case with SCM PR, then we will revert everything and wait for the better PR.

Do you think we need to rephrase it somehow to make it more clear?

doc/pull-requests.md

akosyakov · 2019-07-24T20:15:49Z

@thegecko thank for looking at it, I've applied your suggestions

akosyakov · 2019-07-28T11:29:29Z

@marcdumais-work @svenefftinge please have a look whether you happy with the current state

marcdumais-work · 2019-07-29T11:13:23Z

doc/pull-requests.md

+## Reviewing
+
+<a name="reviewing-template"></a>
+- [1.](#eviewing-template) Reviewers should check that a PR has a [proper description](#pr-template).


Suggestion: also make it clear that the commit(s) should have a proper description (at minimum equivalent to What it does from the PR template. Other info from filled-in template could be nice-to-have as well). This is missed sometimes and makes it harder to understand what a commit does without digging-in the code.

Maybe this (requirement to have meaningful commit message) would fit better in the "review checklist" section, around points number 8 / 9

@marcdumais-work i think it should go to review checklist, could you suggest wording.

Also @benoitf suggested to use conventional commits. I think it is a good idea in the sense that we should automate as much as possible from this checklist. With conventional commits we will need only ask to provide proper commit messages and then we can generate changelogs from them. I know that electon follow such approach. It could be worth to look into after landing this PR.

could you suggest wording.

10. Each commit has meaningful title and a body that explains what it does. One can take inspiration from the "what it does" section from the PR.

Also @benoitf suggested to use conventional commits. [...] It could be worth to look into after landing this PR.

👍

marcdumais-work

Added a suggestion about meaningful commit message for consideration.
Very nice, thanks for putting this together @akosyakov

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

we agreed to follow it

akosyakov · 2019-08-02T07:20:50Z

pinged Sven offline, he said that he is fine to follow it, merging

akosyakov force-pushed the ak/pr_template branch from 8e1f071 to 75bb444 Compare July 1, 2019 09:43

akosyakov requested review from marcdumais-work, paul-marechal, svenefftinge, thegecko, benoitf, tsmaeder and evidolob July 1, 2019 09:44

tsmaeder previously requested changes Jul 1, 2019

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

svenefftinge reviewed Jul 1, 2019

View reviewed changes

akosyakov force-pushed the ak/pr_template branch 2 times, most recently from 9aaf72e to 37db60a Compare July 2, 2019 09:59

kittaakos reviewed Jul 2, 2019

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

This was referenced Jul 3, 2019

Allow not persistent changes for task before run #5312

Closed

Fix plugin loading to cope with modules that have immutable exports #5520

Merged

akosyakov force-pushed the ak/pr_template branch 2 times, most recently from c482258 to 62301da Compare July 4, 2019 07:24

akosyakov requested review from svenefftinge, kittaakos and tsmaeder July 4, 2019 07:28

improve PR template

4b093d7

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/pr_template branch from 62301da to 4b093d7 Compare July 4, 2019 07:30

kittaakos approved these changes Jul 4, 2019

View reviewed changes

svenefftinge reviewed Jul 4, 2019

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

akosyakov mentioned this pull request Jul 8, 2019

fix #5656: Fixed the position of the button on Notification when displaying multiple lines of text #5657

Merged

akosyakov added the quality issues related to code and application quality label Jul 8, 2019

akosyakov force-pushed the ak/pr_template branch 3 times, most recently from e44228a to 73229b2 Compare July 18, 2019 13:52

akosyakov force-pushed the ak/pr_template branch 2 times, most recently from a2bdb69 to 5414a3b Compare July 18, 2019 15:16

vince-fugnitto approved these changes Jul 19, 2019

View reviewed changes

thegecko approved these changes Jul 19, 2019

View reviewed changes

akosyakov force-pushed the ak/pr_template branch from 5414a3b to adab776 Compare July 24, 2019 20:14

marcdumais-work reviewed Jul 29, 2019

View reviewed changes

marcdumais-work approved these changes Jul 29, 2019

View reviewed changes

[dev] adding pull requests docs

4b0b4b4

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/pr_template branch from adab776 to 4b0b4b4 Compare July 29, 2019 19:52

akosyakov merged commit 796d642 into master Aug 2, 2019

akosyakov deleted the ak/pr_template branch August 2, 2019 07:21

magnologan mentioned this pull request Jan 9, 2023

[Snyk] Security upgrade puppeteer from 2.1.1 to 3.0.0 magnologan/theia#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve PR reviews #5616

improve PR reviews #5616

akosyakov commented Jul 1, 2019 •

edited

Loading

tsmaeder commented Jul 1, 2019

kittaakos left a comment

akosyakov commented Jul 4, 2019

akosyakov commented Jul 4, 2019

kittaakos left a comment

benoitf commented Jul 4, 2019

akosyakov commented Jul 4, 2019 •

edited

Loading

benoitf commented Jul 4, 2019

akosyakov commented Jul 18, 2019

vince-fugnitto left a comment

thegecko left a comment

thegecko Jul 19, 2019

akosyakov Jul 24, 2019

akosyakov Jul 24, 2019

akosyakov Jul 24, 2019

akosyakov commented Jul 24, 2019

akosyakov commented Jul 28, 2019

marcdumais-work Jul 29, 2019

marcdumais-work Jul 29, 2019 •

edited

Loading

akosyakov Jul 29, 2019

marcdumais-work Jul 29, 2019

marcdumais-work left a comment

akosyakov commented Aug 2, 2019

improve PR reviews #5616

improve PR reviews #5616

Conversation

akosyakov commented Jul 1, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

tsmaeder commented Jul 1, 2019

kittaakos left a comment

Choose a reason for hiding this comment

akosyakov commented Jul 4, 2019

akosyakov commented Jul 4, 2019

kittaakos left a comment

Choose a reason for hiding this comment

benoitf commented Jul 4, 2019

akosyakov commented Jul 4, 2019 • edited Loading

Review checklist

benoitf commented Jul 4, 2019

akosyakov commented Jul 18, 2019

vince-fugnitto left a comment

Choose a reason for hiding this comment

thegecko left a comment

Choose a reason for hiding this comment

thegecko Jul 19, 2019

Choose a reason for hiding this comment

akosyakov Jul 24, 2019

Choose a reason for hiding this comment

akosyakov Jul 24, 2019

Choose a reason for hiding this comment

akosyakov Jul 24, 2019

Choose a reason for hiding this comment

akosyakov commented Jul 24, 2019

akosyakov commented Jul 28, 2019

marcdumais-work Jul 29, 2019

Choose a reason for hiding this comment

marcdumais-work Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov Jul 29, 2019

Choose a reason for hiding this comment

marcdumais-work Jul 29, 2019

Choose a reason for hiding this comment

marcdumais-work left a comment

Choose a reason for hiding this comment

akosyakov commented Aug 2, 2019

akosyakov commented Jul 1, 2019 •

edited

Loading

akosyakov commented Jul 4, 2019 •

edited

Loading

marcdumais-work Jul 29, 2019 •

edited

Loading