openerp-community team mailing list archive

[Leonardo Pistone <leonardo.pistone@xxxxxxxxxxxxxx>]

Hi Sandy,

Thanks a log for bringing up the problem of frustration by newcomers.
There are indeed things we can do to ease the barrier of entry.

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.

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.

Let me spend a few words about the part of the discussion I really
care about. I am really enthusiastic that we have infrastructure up
and running that checks lint because that does well a part of the job
and allows us to concentrate on the important part: doing code review
as a human activity, to find out if code is understandable, well
structured and so on.

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?".

Thanks to all and sorry for the long mail.
L