www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Assault & battery on pull requests

reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
I've been looking at 6000+ messages about dlang pull requests and their 
associated chatter during the past couple of days. While at it I pulled 
a bunch of requests, and by this I'm asking you to join me.

I have accumulated a bunch of insight in short term memory, among which 
(1) people are happy to work on a variety of things if given just a 
little vision and guidance; (2) it is more difficult yet more productive 
in the long run to shepherd a weak submission with good parts to 
completion, than to dismiss it for its defects; (3) being able to drain 
the request pipeline effectively is key to D's success.

Please join me in reviewing submissions to 
https://github.com/D-Programming-Language/. It doesn't matter if you're 
on the commit team - you only need to be on github, and any meaningful 
review matters. This is much more important, and offers a lot more 
leverage, than spending the same time posting on the forum.

After my review binge, Phobos open pull requests are 44, a historical 
low. Please hop on and let's drain that pipe entirely!


Thanks,

Andrei
Mar 16 2014
next sibling parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Sunday, 16 March 2014 at 21:46:09 UTC, Andrei Alexandrescu 
wrote:

 Thanks,

 Andrei


True dat. Every reviewer helps, and keeping the pipeline low 
helps. In particular, it creates a sinergy where the better the 
faster we pull, the faster quality submissions come in, which is 
awesome.

----

But.... To all the "expert reviewers" out there (myself 
included): Let's try to be "productive" in our reviews, and try 
to keep a broader "big picture" view on the pulls.

1° First: Check the design is correct.
2° THEN: Check the implementation is correct.
3° FINALLY: (optionally) Check the style is correct.

Doing this in the wrong order usually means wasted effort, and/or 
extra useless noise in the review. Furthermore, it can aggravate 
the original submitter, who went through the effort of fixing 
every implementation detail, just see his work rejected because 
of the concept (for example, the "tee" range...).

And go easy on style. The result noise of arguing over style 
usually outweighs its benefits. If it works, and doesn't break 
the style rules, then let's just merge and move on (IMO).
Mar 16 2014
parent reply "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Sunday, 16 March 2014 at 22:10:50 UTC, monarch_dodra wrote:

 But.... To all the "expert reviewers" out there (myself 
 included): Let's try to be "productive" in our reviews, and try 
 to keep a broader "big picture" view on the pulls.

 1° First: Check the design is correct.
 2° THEN: Check the implementation is correct.
 3° FINALLY: (optionally) Check the style is correct.

 Doing this in the wrong order usually means wasted effort, 
 and/or extra useless noise in the review. Furthermore, it can 
 aggravate the original submitter, who went through the effort 
 of fixing every implementation detail, just see his work 
 rejected because of the concept (for example, the "tee" 
 range...).


Certain members with commit rights are really trigger happy. I 
always feel like in a rush to point out any and all issues I can 
see ASAP because otherwise it might be hastily merged.

I'd like to thank you for leaving a PR open for a couple of days 
before merging after having reviewed it yourself. Even if this 
practice was widely adopted though, the problem remains with PRs 
sometimes being merged even when there are still outstanding 
objections.


 And go easy on style. The result noise of arguing over style 
 usually outweighs its benefits. If it works, and doesn't break 
 the style rules, then let's just merge and move on (IMO).


I agree, but it also needs to be recognized that there is a line 
between pure style and objective improvements, like clearer 
variable naming or less sloppily written documentation. I like to 
think that picking on the latter pays off by raising the bar so 
that next time, the author will be keener to such issues.
Mar 16 2014
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/16/14, 3:28 PM, Jakob Ovrum wrote:

 On Sunday, 16 March 2014 at 22:10:50 UTC, monarch_dodra wrote:
 I'd like to thank you for leaving a PR open for a couple of days before
 merging after having reviewed it yourself. Even if this practice was
 widely adopted though, the problem remains with PRs sometimes being
 merged even when there are still outstanding objections.


We can always undo merges.

Andrei
Mar 16 2014
parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Sun, 16 Mar 2014 19:47:39 -0400, Andrei Alexandrescu  
<SeeWebsiteForEmail erdani.org> wrote:


 On 3/16/14, 3:28 PM, Jakob Ovrum wrote:

 On Sunday, 16 March 2014 at 22:10:50 UTC, monarch_dodra wrote:
 I'd like to thank you for leaving a PR open for a couple of days before
 merging after having reviewed it yourself. Even if this practice was
 widely adopted though, the problem remains with PRs sometimes being
 merged even when there are still outstanding objections.


 We can always undo merges.


We currently have "auto merge" feature.

What about an "auto merge after 2 days without further objection" feature?

The problem I see with not merging right away is people forget to do it. I  
would include myself as someone who does that.

Doesn't have to be used in every case, obviously.

-Steve
Mar 18 2014
prev sibling next sibling parent reply Martin Nowak <code dawg.eu> writes:
On 03/16/2014 10:46 PM, Andrei Alexandrescu wrote:

 I've been looking at 6000+ messages about dlang pull requests and their
 associated chatter during the past couple of days. While at it I pulled
 a bunch of requests, and by this I'm asking you to join me.


Thanks a lot.


 I have accumulated a bunch of insight in short term memory, among which
 (1) people are happy to work on a variety of things if given just a
 little vision and guidance; (2) it is more difficult yet more productive
 in the long run to shepherd a weak submission with good parts to
 completion, than to dismiss it for its defects; (3) being able to drain
 the request pipeline effectively is key to D's success.


I came to the same conclusion, we're currently loosing to many useful work.
Mar 16 2014
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/16/14, 3:20 PM, Martin Nowak wrote:

 On 03/16/2014 10:46 PM, Andrei Alexandrescu wrote:

 I have accumulated a bunch of insight in short term memory, among which
 (1) people are happy to work on a variety of things if given just a
 little vision and guidance; (2) it is more difficult yet more productive
 in the long run to shepherd a weak submission with good parts to
 completion, than to dismiss it for its defects; (3) being able to drain
 the request pipeline effectively is key to D's success.


 I came to the same conclusion, we're currently loosing to many useful work.


It's amazing how this puts discussions in perspective. There's all this 
hand wringing about what to do to help D move forward, yet this obvious 
lever has been in front of us all along.

Andrei
Mar 16 2014
prev sibling parent "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Sunday, 16 March 2014 at 21:46:09 UTC, Andrei Alexandrescu 
wrote:

 I have accumulated a bunch of insight in short term memory, 
 among which (1) people are happy to work on a variety of things 
 if given just a little vision and guidance; (2) it is more 
 difficult yet more productive in the long run to shepherd a 
 weak submission with good parts to completion, than to dismiss 
 it for its defects; (3) being able to drain the request 
 pipeline effectively is key to D's success.



is a beginner to either D, dmd/druntime/Phobos or the review 
process; we should aim to foster a culture where the barrier to 
entry is as low as possible. It's always nice to see more 
returning submitters.


 After my review binge, Phobos open pull requests are 44, a 
 historical low. Please hop on and let's drain that pipe 
 entirely!


Thanks for the reviews. It would be really awesome if we could 
get the queue under control. I feel we are heading there.
Mar 16 2014