Assault & battery on pull requests

[Jakob Ovrum]

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.