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 : [email protected] > Unsubscribe : https://launchpad.net/~openerp-community > More help : https://help.launchpad.net/ListHelp >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Mailing list: https://launchpad.net/~openerp-community Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
