openerp-community team mailing list archive

[Houssine BAKKALI <houssine.bakkali@xxxxxxxxx>]

why not just to add an entry in the manifest to say which model has been
inherited/modified as it AFAIU the main reason raised.

splitting the modification in files by model make sense for xml but for py
file as far as they contain a bug number of lines of code we won't gain any
benefit.

my 2 cents...

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