openerp-community team mailing list archive

[Sandy Carter <sandy.carter@xxxxxxxxxxxxxxxxxxxx>]

Le 2014-10-22 04:09, Leonardo Pistone a écrit :

> I think we should wipe the document, and start entering the
> conventions that are already accepted, that is those that we already
> check with flake8 and pylint.

+1

> On the topic of one-file-per-class, I understand the reasons, but I
> vote -1 because it seems too restricting to me, and I see many good
> quality python projects not doing so. Still, I am not fighting on
> that. I consider it a minor point.

I would just like to clarify that I am not asking for
one-file-per-class, but one-file-per-model.

Helper classes should be where it is most logical to put them.

What makes the case of Odoo different from some other python projects is
that in 90% of our cases we deal with ORM models and not purely Object
Oriented classes. Usual workflow consists of altering one or multiple
models. In my opinion, having them separate improves readability and
makes maintenance easier.

Some are suggesting we do the one-file-per-model as a "Nice to have".
Have no problem with that, but I'd rather call it a "Strong
recommendation". ;)

> Take for example any of the huge python files in the core, like
> account.py. Imagine it going through our code review.
> 
> The first pass would be automatic: we would get rid of unused
> variables, long lines and so on. We could ask to split the classes in
> smaller files. That is great but it is just the start.
> 
> Then we would get to the real code review, where humans say things like:
> - what are __compute(), res, res2, ids, ids2? What does that mean? Can
> you refactor?
> - why a class with 43 methods? maybe we need a service class, or a mixin?
> - the docstring says that "context" is a context, and we know that.
> Instead, describe what the method if it is not clear from the name.
> - this duplicates code from class XY. can we make it more orthogonal?
> - I just do not understand at all what this method is about. can you
> clarify / refactor / document?
> 
> And so on. The same of course goes for tests: it is a great and
> necessary start that the machine tells us the coverage, but then our
> review will say, for example "this test does not reflect the use case
> in the manifest, can you explain?".

+1 to this, I think we could reformulate this to fit it into the
guidelines document to, at least, explain the process and what we
reviewers should look for.

Attachment: signature.asc
Description: OpenPGP digital signature