openerp-community team mailing list archive

[Sandy Carter <sandy.carter@xxxxxxxxxxxxxxxxxxxx>]

Hi,

My issue with leaving it to subjective criticism is:

Firstly, if you're new and learning the ropes to contribution to OCA and
you do your homework, you're going to do two things to make sure your
module gets integrated. First you'll check existing modules and emulate
what they do and second you'll read the guidelines. Like I mentioned at
the beginning, there are many cases of X models in files named like
'product.py', and if you look at the guidelines[1], it is still bare in
many parts.

We see this today, a new dev will do her work, prepare her module using
these guidelines. When he finally does a pull request, we come down on
him with these subjective reviews, that we are used to, but to the dev,
these seem completely arbitrary. The result is often that the new dev
will redo her module multiple times with no more guidelines than the
review of a select few contributors who have had the time to review her
code. Another possible outcome, is that these PRs get stuck in limbo
where the dev might have moved on to another project.

A second problem with subjective criticism is when a disagreement is met
or a dev making a PR is unwilling to change his code and chooses to
argue every point.

Without the points in the review page, a subjective review has very
little weight, you can't just point at the page and say: "These are the
standards we do our review by, this is what everyone who contributes
agrees to for the sake of uniform coding style". Instead we find
ourselves saying: "This is better because it is", "This is what is done
in most modules", "This is not yet on the page".

These two situations happen quite often. I have multiple less
experienced Odoo developers come to me frustrated because their PRs are
being stalled for what seems to them to be arbitrary reasons. The first
thing they ask is usually "What does she mean by X? This seems
unreasonable." I often reply with "That kind of sucks but I agree with
his review". the second thing they ask is "Where can I find all these
guidelines so I can avoid being stalled on getting my code merged?" And
for that, I would love to be able to refer to the guidelines page[1]. At
the present, I can't really...

What the consensus I seem to get out of this is: Often it is better to
separate the models (especially in xml), but there are some acceptable
exceptions:

* Small changes to models
  * One to many relationships (single column add to related model)
  * At some point, a small change becomes to big and the file must be split.
* All changes are very closely related to a same feature
* Wizards
* Connectors (I'm adding this one)

No one seemed to object to naming the Class, .py file and xml files
according to the model (or largest model in the case of filenames).

Is this something we can agree on?

[1] http://odoo-community.org/page/website.how-to

Le 2014-10-20 05:39, Leonardo Pistone a écrit :
> Hi all,
> 
> I understand the convenience of finding classes easily, and the
> reasons behind this are sound. Still, I do not sopport such a rule on
> splitting files.
> 
> Still, I think we can and should work to improve subjective quality
> and clarity of code. Example: after the machine checked lint, I think
> we can (and should) gradually move to comments like
> 
> - this method / this file is long, can you split it?
> - I don't understand what this is for, can you comment or refactor?
> - why do we need that? (comment or refactor)
> - (and one day, gradually...) could you decouple this logic from the
> database schema to a pure class?
> 
> TL;DR I oppose a rule on file length and such, but I favor subjective
> criticism to get the code as clear as possible.
> 
> Thanks to all for the involvement!
> 
> On 10/19/2014 10:46 PM, Graeme Gellatly wrote:
> But for me this comes
>> down to personal preference, not sure it needs to be a convention.
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp
>

Attachment: signature.asc
Description: OpenPGP digital signature