[Bf-python] Handling cleanup patches from new contributor

[Bastien Montagne]

I have two problems with those patches:

I) I don’t really see the point here. Most of those changes are mere 
matter of tastes, they do not give many (if any at all) speedup etc. 
They are not bad in themselves, just don’t see the point on wasting time 
reviewing, applying, testing, handling possible issues (typos or worse, 
see below) afterwards, etc.

II) Some of those changes are potentially dangerous if not *very* 
carefully triple-checked. For example, replacing 'a = a + 1' by 'a += 1' 
is on first sight a mere matter of taste. *BUT* those two expressions 
are not the same thing, not with mutable types, see 
http://stackoverflow.com/questions/2347265/why-does-behave-unexpectedly-on-lists/2347423#2347423 
!

We had a nice guy, knowing Blender codebase much better than pretty much 
any of us, who used to do large cleanups like that - how often did we 
had to fix some stupid typo, forgotten pieces of code, etc. afterwards - 
often even fixing broken build? And Blender is much, much more built and 
tested on daily basis than its addons. What I mean here is: cleanup *is 
not* a trivial activity, not even when you are an expert in both the 
language and the project.

So my point is: leave cleanups to addons' authors/maintainers. We have a 
lot (too much) to do already with bug reports and patch reviews, 
including in addon area… I’m sorry for the author of those patches, but 
I personally don’t feel like taking some time to go over them in detail, 
carefully checking they do not break/change behaviors unexpectedly, just 
for sake of some slightly better looking code.

I don’t mind if someone takes time to do this - but I’d like to stress 
this: please only consider changes on 'OFFICIAL' addons, the others we 
should only limit ourselves to critical bugfixes, period.



Le 08/09/2016 à 17:22, Christopher Barry a écrit :
> Is it possible to merge the patches into a branch and ask on the list
> for people familiar with the addons the patch addresses to try the
> branch and test the changes?
>
> If all goes well, e.g. no complaints after a month or so, just merge
> that into master.
>
> Getting people to actively investigate and cleanup other's code is
> probably not easy to do. Don't alienate this angel :)
>
> -C
>
>
> On Thu, 8 Sep 2016 17:03:48 +0200
> Sergey Sharybin <sergey.vfx at gmail.com> wrote:
>
>> Hi,
>>
>> There are some reasonable changes from (a) pep8 point of view (which
>> we're trying to stick anyway) and (b) performance point of view.
>> Rejecting such patches seems weird to me.
>>
>> Also, instead of rejecting patches and still asking maintainers to
>> check on something is even more stupid. If we want to go this route,
>> just ask the guy to do per-addon changes and assign those to
>> maintainers.
>>
>> Or simply go ahead and apply reasonable patches globally on the whole
>> scripts folder.
>>
>>
>>
>> On Thu, Sep 8, 2016 at 3:07 PM, Julian Eisel <eiseljulian at gmail.com>
>> wrote:
>>
>>> Hi all,
>>>
>>> Today a new contributor submitted a bunch of cleanup patches for
>>> Add-ons (D2201, D2203, D2204, D2205, D2206, D2207, D2208, D2209,
>>> D2210). This moves us in a kinda stupid situation: While we don't
>>> want to disappoint a contributor (a new one, who probably put quite
>>> some effort into the patches), we also don't want to spend much time
>>> reviewing/testing biggish cleanup patches with basically zero
>>> benefit, except of consistent code style (though I agree that this
>>> has its importance too). Especially now that Campbell isn't
>>> available to help anymore.
>>> It's also our general policy to let Add-on maintainers handle fixes
>>> and cleanups as much as possible.
>>>
>>> So as said, this is a kinda stupid situation and we're not sure how
>>> to solve it. I'd propose we reject the patches, but use this mailing
>>> list to ask Add-on maintainers to have a look at the patches. They
>>> can then review and merge the changes that apply to their Add-ons
>>> (with proper credits please).
>>>
>>> We discussed this briefly in #blenderpython and agreed on rejecting
>>> the patches (after all, author could have contacted us earlier too),
>>> but I decided to write this quick mail to avoid frustrated
>>> contributors.
>>>
>>> Cheers,
>>> - Julian -
>>> _______________________________________________
>>> Bf-python mailing list
>>> Bf-python at blender.org
>>> https://lists.blender.org/mailman/listinfo/bf-python
>>>   
>>
>>
>
>