[scribus-dev] patches are ready...

[Craig Bradney]

On 10/2/12 8:29 PM, Craig Bradney wrote:
> On 10/2/12 6:59 PM, a.l.e wrote:
>> hi greg,
>>> I guess I would tend to line up with Craig about being conservative about
>>> incorporating patches. It's one thing if a patch works, but one wants to
>>> know if it risks or actually does break something else, and there has to
>>> be a desire to keep some consistency in the way things are implemented in
>>> the code.
>>
>> i've never meant to say that every patch must be accepted with no consideration of its quality.
>>
>> but the contributor deserves a fast answer: accepted or refused.
>> and if refused, what has to be improved (if it can be improved...)
> 
> I'd agree where I'd qualify "fast" as "when we can review it". Small patches take a couple of
> minutes. Massive patches like Cezary's footnotes patch could take weeks (where week=week of natural
> life rather than dedicated time). Time I give to Scribus these days might be as little as a couple
> of hours a week.
> 
> Patches should *only* change one area at a time, not, eg, adding footnotes and indic support or
> something like that.
> 
> Also, it is unrealistic, given the above definition of week and paid vs interest-based work on
> Scribus, that a large patch will be reviewed line for line. There is therefore the expectation that
> a submitted-for-commit patch would be ready to go in terms of functional testing being complete.
> 
> Therefore, a contributor should do work in a git branch and ask people to review it there for bugs
> and functionality gaps. Once complete, then a bug should be submitted with reviewers agreeing it has
> been tested and should go into SVN. Those who want to work but don't want to use git should pass a
> working patch to reviewers directly for testing.
> 
> People who want code style or code review at any point in the process earlier than patch submission
> can happily send a patch via email or via a bug, assuming the qualification is made that its for
> code review rather than for committing,
> 

To add to this. and WITHOUT picking on Cezary.. but using this as an example as its a good way to
learn, from #11050:

- "Initial code for autonumbering paragraphs is included, but not finished."

* autonumbering should be separated unless its intrinsically linked to the main patch
* its not finished, so don't include it, or don't even submit the patch

- "But BULLETS seems to working with simply support of read them from ODT files."

* Maybe this is just a translation issue, but "seems to be working" is a bad description for
something to be committed.

Craig