openerp-expert-framework team mailing list archive

[Alexandre Fayolle <alexandre.fayolle@xxxxxxxxxxxxxx>]

On 17/04/2012 13:24, Olivier Dony wrote:
> On 04/16/2012 07:37 PM, Valentin LAB wrote:
>> Is there any coding convention enforced on the OpenERP Code ?
> As Lionel (Numérigraphe) said, we try to enforce common-sense best practices
> when working on new developments, refactoring existing code, and every
> opportunity we have to improve code.
>
> However, we prefer to avoid changing unimportant things like whitespace,
> line-wrapping etc when we don't have any other reason to touch the code.
> In general it has little value compared to the cost of decreasing the
> visibility of the main goal of the patch, creating extraneous merge conflicts,
> and even causing unexpected regressions.
>
> In rare cases it may make sense to do a quick mass-cleanup, if and when the
> timing is convenient.
>
>
>> I'm thinking about:
>> - code pure aesthetic as the PEP08, or PEP257
>>     http://www.python.org/dev/peps/pep-0008/
>>     http://www.python.org/dev/peps/pep-0257/
>>     (ie: trailing whitespace, 80 col ...)
> Those PEPs are very good but we don't enforce them to the letter, common sense
> always prevails [1]. As an example, I will never hold it against any developer
> if they don't strictly wrap their line at 80 chars, because we don't have good
> reasons to enforce it. When was the last time you _really_ needed 3 files
> side-by-side on the same screen? OTOH that wrapping length flexibility usually
> improves readability (e.g. 90 cols are fine if it avoids wrapping at all)

What we want is to avoid code which common sense knows is wrong (200+
char long lines...). So having a recommendation is always useful. After
that, the recommended length is somewhat arbitrary. Setting a shorter
limit encourages reasonable variable, method  and field names, which is
a plus when you have to type them, as well as when you have to read them
(for having two 30-char long field names differing only by a couple of
letters somewhere in the middle does not ease maintenance)


>> - best practice as checked by pylint
>>     (size of variables, number of method, ...)
>> - code coverage checks
>> - code complexity checks
> For these kinds of improvements we definitely welcome patches, such as
> refactoring of over-sized methods, dead code removal, etc. But one has to keep
> in mind that there's always a risk, these tools only give hints...

That's why we need the runbot back :-)

And more automated tests :-)

> I hope this gives you a general idea of what to do when something is bugging
> you in the source code... ;-)
>
>

For the record, I have a patch for the osv.osv -> orm.Model renaming
(discussed during the conf with some openerp folks). I'm waiting for
runbot to resuscitate to run the tests before sending the merge
requests. I have a few others cleanup patches planned. I'll try to
announce them on this list to avoid duplicating work.

-- 
Alexandre Fayolle
Chef de Projet
Tel : + 33 (0)4 79 26 57 92

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac Cedex
http://www.camptocamp.com