A note about pull requests and reviews

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

A note about pull requests and reviews

David M. Lloyd
A quick note about pull requests, particularly those with multiple
commits - not just on WildFly but on *all* projects.

In order to efficiently review a pull request, especially a complex one,
it *must* be possible to review it one commit at a time.  This means
that if there's a review item on your pull request for a mistake or
problem, don't just add a commit to the PR to fix it.  Rather, please
amend or remove the faulty commit completely, otherwise some other
reviewer who is looking one commit at a time is just going to waste time
reporting the same problem only to have to go back and remove it once
the later commit found.

Remember: PR *submitters* have a great deal more processing power than
PR *reviewers*.

Thanks!
--
- DML
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: A note about pull requests and reviews

David M. Lloyd
On 11/17/2016 08:47 AM, David M. Lloyd wrote:

> A quick note about pull requests, particularly those with multiple
> commits - not just on WildFly but on *all* projects.
>
> In order to efficiently review a pull request, especially a complex one,
> it *must* be possible to review it one commit at a time.  This means
> that if there's a review item on your pull request for a mistake or
> problem, don't just add a commit to the PR to fix it.  Rather, please
> amend or remove the faulty commit completely, otherwise some other
> reviewer who is looking one commit at a time is just going to waste time
> reporting the same problem only to have to go back and remove it once
> the later commit found.
>
> Remember: PR *submitters* have a great deal more processing power than
> PR *reviewers*.

And in addition - very large sweeping changes are best broken down into
multiple commits whenever possible for this reason!
--
- DML
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: A note about pull requests and reviews

Brian Stansberry
In reply to this post by David M. Lloyd
+1. Fixing problems in the original commit also makes maintenance easier. Five years from now someone you may never meet is going to want to understand your change and it’s more likely they’ll get it right if incorrect bits in the first attempt are invisible.

> On Nov 17, 2016, at 8:47 AM, David M. Lloyd <[hidden email]> wrote:
>
> A quick note about pull requests, particularly those with multiple
> commits - not just on WildFly but on *all* projects.
>
> In order to efficiently review a pull request, especially a complex one,
> it *must* be possible to review it one commit at a time.  This means
> that if there's a review item on your pull request for a mistake or
> problem, don't just add a commit to the PR to fix it.  Rather, please
> amend or remove the faulty commit completely, otherwise some other
> reviewer who is looking one commit at a time is just going to waste time
> reporting the same problem only to have to go back and remove it once
> the later commit found.
>
> Remember: PR *submitters* have a great deal more processing power than
> PR *reviewers*.
>
> Thanks!
> --
> - DML
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Loading...