How to review pull requests
10 views
Skip to first unread message
Antonin Delpeuch (lists)
May 27, 2022, 3:44:40 AM5/27/22
to openref...@googlegroups.com
Hello all,
Some time ago, Antoine B. asked me for some feedback about how he
reviewed PRs. So I thought a bit about how I try to review PRs, and I am
summarizing it here for discussion, because I think it is a very
interesting question.
First, let us state the obvious: I try to be nice to the PR author.
Making a PR can be quite a lot of work, for which I am grateful, so I
want the PR reviewing process to be as swift and pleasant as possible.
Ideally, that means responding quickly, with comments that are relevant
and which help the author improve.
A pull request can have all sorts of problems, with various degrees of
importance. I try to bring up the most important problems first, because
it is frustrating for the author to work on small details if the PR ends
up being blocked later on by something bigger. I often fail to do that.
That happens when I want to give feedback too quickly. Indeed, when a
new PR lands, it is tempting to have a look at the code changes and
pinpoint some indentation or code style issues, because those are quite
easy to see. So when I am in a rush, I often fall for those shortcuts,
but that is bad.
How to prioritize the problems to bring up? Here is an attempt at a
rough prioritization.
- If the PR is tackling an existing issue, is this issue actually valid?
Maybe the issue author misunderstood how the tool is meant to work. Or
the issue has been fixed already by another change. And so on. It does
sometimes happen that people open PRs for such stale issues, for various
reasons.
- if the PR is introducing a new dependency, is it a legitimate one? I
try to check for: the size of the library, its license, if it is
actively maintained, if there are any red flags about its quality, if
there are any alternatives to it and whether those could be more suitable.
- are there relevant tests? Some changes are easier to test than others,
it is important not to have unreasonably high expectations about
automated testing (I tend to be lazier about testing when I write code
myself!). Do those tests pass and do they look sensible?
- does it actually work? Testing the change manually is obviously
important, although I have to admit I skip that for trusted contributors
or obvious fixes. I do tend to do this manual testing after checking for
unit / integration tests, although those two steps should ideally be
swapped I suppose. It is just much easier to see if a PR includes tests
or not, so I often fall for that.
- is there anything off about the code itself? From structural problems
to style issues, there is a wide range of things that can go wrong.
- is there accompanying documentation changes? Not all code changes
require documentation changes though.
Those are just things on top of my mind, I am pretty sure I must be
forgetting about some things. The point is: there is no point in asking
for a documentation update if the dependency we are pulling in has an
incompatible license. As a PR author, it is frustrating to respond with
the required changes if the PR ends up being blocked by something bigger.
What do you think? How do you review PRs, in OpenRefine or elsewhere?
What can I improve in my reviewing?
Cheers,
Antonin
Some time ago, Antoine B. asked me for some feedback about how he
reviewed PRs. So I thought a bit about how I try to review PRs, and I am
summarizing it here for discussion, because I think it is a very
interesting question.
First, let us state the obvious: I try to be nice to the PR author.
Making a PR can be quite a lot of work, for which I am grateful, so I
want the PR reviewing process to be as swift and pleasant as possible.
Ideally, that means responding quickly, with comments that are relevant
and which help the author improve.
A pull request can have all sorts of problems, with various degrees of
importance. I try to bring up the most important problems first, because
it is frustrating for the author to work on small details if the PR ends
up being blocked later on by something bigger. I often fail to do that.
That happens when I want to give feedback too quickly. Indeed, when a
new PR lands, it is tempting to have a look at the code changes and
pinpoint some indentation or code style issues, because those are quite
easy to see. So when I am in a rush, I often fall for those shortcuts,
but that is bad.
How to prioritize the problems to bring up? Here is an attempt at a
rough prioritization.
- If the PR is tackling an existing issue, is this issue actually valid?
Maybe the issue author misunderstood how the tool is meant to work. Or
the issue has been fixed already by another change. And so on. It does
sometimes happen that people open PRs for such stale issues, for various
reasons.
- if the PR is introducing a new dependency, is it a legitimate one? I
try to check for: the size of the library, its license, if it is
actively maintained, if there are any red flags about its quality, if
there are any alternatives to it and whether those could be more suitable.
- are there relevant tests? Some changes are easier to test than others,
it is important not to have unreasonably high expectations about
automated testing (I tend to be lazier about testing when I write code
myself!). Do those tests pass and do they look sensible?
- does it actually work? Testing the change manually is obviously
important, although I have to admit I skip that for trusted contributors
or obvious fixes. I do tend to do this manual testing after checking for
unit / integration tests, although those two steps should ideally be
swapped I suppose. It is just much easier to see if a PR includes tests
or not, so I often fall for that.
- is there anything off about the code itself? From structural problems
to style issues, there is a wide range of things that can go wrong.
- is there accompanying documentation changes? Not all code changes
require documentation changes though.
Those are just things on top of my mind, I am pretty sure I must be
forgetting about some things. The point is: there is no point in asking
for a documentation update if the dependency we are pulling in has an
incompatible license. As a PR author, it is frustrating to respond with
the required changes if the PR ends up being blocked by something bigger.
What do you think? How do you review PRs, in OpenRefine or elsewhere?
What can I improve in my reviewing?
Cheers,
Antonin
Antoine Beaubien
May 30, 2022, 8:26:53 PM5/30/22
to OpenRefine Development
Thank you very much Antonin for taking the time to write this.
Very appreciated.
Regards,
Antoine
Tom Morris
Nov 24, 2022, 5:38:44 PM11/24/22
to openref...@googlegroups.com
Ideally code review practices should be consistent across all reviewers. Has this been codified in a checklist on the wiki? I wasn't able to find anything obvious.
Tom
--
You received this message because you are subscribed to the Google Groups "OpenRefine Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openrefine-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openrefine-dev/964c16aa-dab1-ecc8-4fc8-bdf4338bccc3%40antonin.delpeuch.eu.
Antonin Delpeuch (lists)
Nov 25, 2022, 2:49:44 AM11/25/22
to openref...@googlegroups.com
I think it would make sense to codify that indeed, and I do not think we
have such a document yet. If you feel like starting something, go for it!
I intend to have a go at restructuring and improving the technical
documentation on the website (in an effort to keep migrating out of the
wiki), so it could have its place there.
Antonin
> <mailto:openrefine-dev%2Bunsu...@googlegroups.com>.
have such a document yet. If you feel like starting something, go for it!
I intend to have a go at restructuring and improving the technical
documentation on the website (in an effort to keep migrating out of the
wiki), so it could have its place there.
Antonin
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/openrefine-dev/964c16aa-dab1-ecc8-4fc8-bdf4338bccc3%40antonin.delpeuch.eu <https://groups.google.com/d/msgid/openrefine-dev/964c16aa-dab1-ecc8-4fc8-bdf4338bccc3%40antonin.delpeuch.eu>.
>
> --
> You received this message because you are subscribed to the Google
> Groups "OpenRefine Development" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to openrefine-de...@googlegroups.com
> <mailto:openrefine-de...@googlegroups.com>.
> --
> You received this message because you are subscribed to the Google
> Groups "OpenRefine Development" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to openrefine-de...@googlegroups.com
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/openrefine-dev/CAE9vqEEF1PWVKEUQ7AGRP5iPDykEn7R8QGXJxyUsPx3TZ%2BBkGA%40mail.gmail.com <https://groups.google.com/d/msgid/openrefine-dev/CAE9vqEEF1PWVKEUQ7AGRP5iPDykEn7R8QGXJxyUsPx3TZ%2BBkGA%40mail.gmail.com?utm_medium=email&utm_source=footer>.
Reply all
Reply to author
Forward
0 new messages