Re: When to start patch review

[Simon Albrecht]

On 17.02.2016 09:42, James Lowe wrote:
> On 16/02/16 21:34, Simon Albrecht wrote:
> > Hello,
> >
> > actually it would make sense to only start Rietveld review _after_ 
> > testing, i.e. when the tracker issue is set to Patch:review – 
> > wouldn’t it?
>
> Full make doc errors are always forgivable, as are unexpected reg test 
> diffs, but I do get irked when a patch doesn't even pass make (or 
> cannot even apply to current master) when it is obvious that the dev 
> hasn't really even tried to make sure it works before submission for 
> *full* testing.
>
> The assumption is therefore, (I guess, as someone who doesn't code), 
> that a dev should know what they are doing and have at least run 
> 'make' - which exercises the patch to a certain degree and will catch 
> most of the obvious Texinfo syntax mistakes - before they submit. My 
> full patch testing then just exercises the reg test and the full doc 
> build.
>
> Also, (speaking as *the* person that does 'all' the patch testing - 
> other volunteers are always welcome to chip in BTW), it can save me 
> time if someone has already spotted something obvious before I get 
> round to testing; in that case I won't even bother and just set the 
> patch back to needs_work (with comment on the tracker) - it just 
> depends on how convenient it is for me at the time and if the comments 
> in Rietveld are 'gentle discussion' type comments or not.
>
> For example, if some dev puts up a patch and another dev then looks at 
> that patch and sees that it breaks all the 'conventions' (or whatever 
> term you want to use) with LP's syntax or design philosophy then I may 
> not bother to test.
>
> Waiting for a patch like that to pass before review is pointless if 
> there is going to be a reluctance or even refusal to allow it to make 
> it to master in the end.
>
> However, is there a particular reason for this question? 

No, just a thought I had. And what you say is good, so it’s clear now.

Best, Simon