openerp-community team mailing list archive

[Pedro Manuel Baeza Romero <pedro.baeza@xxxxxxxxx>]

Sandy, for me it's OK this way.

Regards.

2014-10-20 16:52 GMT+02:00 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
> >
>
>
> _______________________________________________
> 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
>
>