D pull request review process -- strawman formal definition, query for tools

[Daniel Murphy]

"Steven Schveighoffer" <schveiguy at yahoo.com> wrote in message 
news:op.wwvee6wleav7ka at stevens-macbook-pro.local...
>> Sure, but this does not ensure that the tests have full coverage of the
>> problem, or even full coverage of the cases presented in the bug report.
>> This probably applies to dmd more than the others where failures can have
>> very complex interacting causes.
>
> Up to your discretion as a reviewer what you require.  It's impossible to 
> formalize this requirement as a documented/objective measure.  I would say 
> that 9 times out of 10, the bug report will contain a minimized example 
> that causes the issue to occur, that should just be included as a 
> unit-test.
>

Ok, I think this might be different for dmd vs phobos.

>
> I didn't consider that the auto-tester wouldn't test that the test case 
> fails on the existing code.  I suppose that would just be up to the 
> reviewer whether you wanted to go through the effort of testing with that 
> test on the current master branch, or eyeball it and assume it does fail. 
> The most important thing is that a unit test is inserted to verify the fix 
> actually works, and that a regression is caught immediately.
>

Absolutely agreed, this is fundamental.  After all it isn't really fixed 
unless you can guarantee it _stays_ fixed.  Adding tests also has the nice 
effect of pushing the burden of keeping it working onto the next person to 
modify the code. :)

>
> That is perfect.  A couple people have mentioned we can do this without 
> github issues, the pull requests support labels and assignment.  Though 
> mutually exclusive labels might be awkward.  Perhaps we should redefine 
> the stages with that in mind.
>

They support labels and assignment because they automatically get an issue 
created for them.  I think you have to turn on issue support in order to 
actually add labels to the pull requests (or at least I can't find the 
interface).